Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580994 > unrolled thread

[PATCH 0/6] x86/platform/uv/BAU: UV4 message completion and initialization updates

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] x86/platform/uv/BAU: UV4 message completion and initialization updates Andrew Banman <abanman@hpe.com> - 2017-02-15 03:00 +0100
    [PATCH 2/6] x86/platform/uv/BAU: Add status_mmr_loc to locate message status bits Andrew Banman <abanman@hpe.com> - 2017-02-15 03:00 +0100
      Re: [PATCH 2/6] x86/platform/uv/BAU: Add status_mmr_loc to locate  message status bits Thomas Gleixner <tglx@linutronix.de> - 2017-02-16 19:10 +0100
    [PATCH 5/6] x86/platform/uv/BAU: Remove initial write to swack register Andrew Banman <abanman@hpe.com> - 2017-02-15 03:10 +0100
      Re: [PATCH 5/6] x86/platform/uv/BAU: Remove initial write to swack  register Thomas Gleixner <tglx@linutronix.de> - 2017-02-16 19:30 +0100
    [PATCH 1/6] x86/platform/uv/BAU: Declare bau_operations struct after other BAU structs Andrew Banman <abanman@hpe.com> - 2017-02-15 03:10 +0100
      Re: [PATCH 1/6] x86/platform/uv/BAU: Declare bau_operations struct  after other BAU structs Thomas Gleixner <tglx@linutronix.de> - 2017-02-16 19:10 +0100

#1580994 — [PATCH 0/6] x86/platform/uv/BAU: UV4 message completion and initialization updates

The following patch series adds the necessary functionality to make the BAU
on UV4 operational. The purpose of these patches is to implement the correct
message completion logic on UV4 and to fix two initialization errors.

As of this patch set, the BAU operates without errors and performance tests
show TLB shootdowns take up to 42% less time with the BAU enabled.

The patches are summarized as follows:

(1) Make the wait_completion routine part of the bau_operations interface,
    and add a uv4_wait_completion routine to employ new completion logic.

    The message completion logic for previous generations relies on software-
    defined timeouts that are not implemented on UV4. Without these patches,
    the BAU driver on UV4 erroneously identifies a UV2-WAR timeout during
    normal operation.

    [PATCH 1/6] x86/platform/uv/BAU: Declare bau_operations struct after
    [PATCH 2/6] x86/platform/uv/BAU: Add status_mmr_loc to locate message
    [PATCH 3/6] x86/platform/uv/BAU: Add wait_completion to bau_operations
    [PATCH 4/6] x86/platform/uv/BAU: Implement uv4_wait_completion with

(2) Fix an initialization error in which writing to the software acknowledge
    clear register causes a HUB error to assert, resulting in an NMI.

    [PATCH 5/6] x86/platform/uv/BAU: Remove initial write to swack

(3) Populate a message payload field to verify messages at the destination.
    Without this verification, the destination agent triggers a HUB error,
    resulting in an NMI.

    [PATCH 6/6] x86/platform/uv/BAU: Add payload descriptor qualifier


Please see the commit messages for details on the motivation and content of
each patch.

Thank you,

Andrew Banman
<abanman@hpe.com>
Linux Kernel Engineer
Hewlett Packard Enterprise

[toc] | [next] | [standalone]

#1580996 — [PATCH 2/6] x86/platform/uv/BAU: Add status_mmr_loc to locate message status bits

The location of the ERROR and BUSY status bits depends on the descriptor
index, i.e. the CPU, of the message. We determine this location ahead of
the wait_completion loop to avoid repeating the calculation.

Split out the status location calculation into a new routine,
status_mmr_loc, to be used within each uv*_wait_completion routine.

Signed-off-by: Andrew Banman <abanman@hpe.com>
Acked-by: Mike Travis <mike.travis@hpe.com>
---
 arch/x86/platform/uv/tlb_uv.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

Index: community/arch/x86/platform/uv/tlb_uv.c
===================================================================
--- community.orig/arch/x86/platform/uv/tlb_uv.c
+++ community/arch/x86/platform/uv/tlb_uv.c
@@ -533,6 +533,22 @@ static inline void end_uvhub_quiesce(str
 	atom_asr(-1, (struct atomic_short *)&hmaster->uvhub_quiesce);
 }
 
+/*
+ * The ERROR and BUSY status registers are located pairwise over the STATUS_0
+ * and STATUS_1 mmrs; each an array[32] of 2 bits. Given CPU desc, determine
+ * the correct mmr and index for the message status.
+ */
+void status_mmr_loc(unsigned long *mmr, int *index, int desc)
+{
+	if (desc < UV_CPUS_PER_AS) {
+		*mmr = UVH_LB_BAU_SB_ACTIVATION_STATUS_0;
+		*index = desc * UV_ACT_STATUS_SIZE;
+	} else {
+		*mmr = UVH_LB_BAU_SB_ACTIVATION_STATUS_1;
+		*index = (desc - UV_CPUS_PER_AS) * UV_ACT_STATUS_SIZE;
+	}
+}
+
 static unsigned long uv1_read_status(unsigned long mmr_offset, int right_shift)
 {
 	unsigned long descriptor_status;
@@ -548,13 +564,16 @@ static unsigned long uv1_read_status(uns
  * return COMPLETE, RETRY(PLUGGED or TIMEOUT) or GIVEUP
  */
 static int uv1_wait_completion(struct bau_desc *bau_desc,
-				unsigned long mmr_offset, int right_shift,
 				struct bau_control *bcp, long try)
 {
 	unsigned long descriptor_status;
+	unsigned long mmr_offset;
+	int right_shift;
+	int desc = bcp->uvhub_cpu;
 	cycles_t ttm;
 	struct ptc_stats *stat = bcp->statp;
 
+	status_mmr_loc(&mmr_offset, &right_shift, desc);
 	descriptor_status = uv1_read_status(mmr_offset, right_shift);
 	/* spin on the status MMR, waiting for it to go idle */
 	while ((descriptor_status != DS_IDLE)) {
@@ -640,15 +659,17 @@ int handle_uv2_busy(struct bau_control *
 }
 
 static int uv2_3_wait_completion(struct bau_desc *bau_desc,
-				unsigned long mmr_offset, int right_shift,
 				struct bau_control *bcp, long try)
 {
 	unsigned long descriptor_stat;
+	unsigned long mmr_offset;
 	cycles_t ttm;
 	int desc = bcp->uvhub_cpu;
+	int right_shift;
 	long busy_reps = 0;
 	struct ptc_stats *stat = bcp->statp;
 
+	status_mmr_loc(&mmr_offset, &right_shift, desc);
 	descriptor_stat = uv2_3_read_status(mmr_offset, right_shift, desc);
 
 	/* spin on the status MMR, waiting for it to go idle */
@@ -712,22 +733,10 @@ static int uv2_3_wait_completion(struct
  */
 static int wait_completion(struct bau_desc *bau_desc, struct bau_control *bcp, long try)
 {
-	int right_shift;
-	unsigned long mmr_offset;
-	int desc = bcp->uvhub_cpu;
-
-	if (desc < UV_CPUS_PER_AS) {
-		mmr_offset = UVH_LB_BAU_SB_ACTIVATION_STATUS_0;
-		right_shift = desc * UV_ACT_STATUS_SIZE;
-	} else {
-		mmr_offset = UVH_LB_BAU_SB_ACTIVATION_STATUS_1;
-		right_shift = ((desc - UV_CPUS_PER_AS) * UV_ACT_STATUS_SIZE);
-	}
-
 	if (bcp->uvhub_version == 1)
-		return uv1_wait_completion(bau_desc, mmr_offset, right_shift, bcp, try);
+		return uv1_wait_completion(bau_desc, bcp, try);
 	else
-		return uv2_3_wait_completion(bau_desc, mmr_offset, right_shift, bcp, try);
+		return uv2_3_wait_completion(bau_desc, bcp, try);
 }
 
 /*

[toc] | [prev] | [next] | [standalone]

#1582775 — Re: [PATCH 2/6] x86/platform/uv/BAU: Add status_mmr_loc to locate message status bits

On Tue, 14 Feb 2017, Andrew Banman wrote:

> The location of the ERROR and BUSY status bits depends on the descriptor
> index, i.e. the CPU, of the message. We determine this location ahead of
> the wait_completion loop to avoid repeating the calculation.
> 
> Split out the status location calculation into a new routine,
> status_mmr_loc, to be used within each uv*_wait_completion routine.

And the reason for this is? You just tell WHAT you are doing, not the WHY.

Looking at the patch which implements the uv4 wait function it uses the
thing as well. So for the casual reader there is no point.

The only reason i figured why you want to do that is to reduce the number
of arguments to the wait function, correct?

If yes, then spell it out. If no, please enlighten me.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1580997 — [PATCH 5/6] x86/platform/uv/BAU: Remove initial write to swack register

Writing to the software acknowledge clear register when there are no
pending messages causes a HUB error to assert. The original intent of this
write was to clear the pending bits before start of operation, but this is
an incorrect method and has been determined to be unnecessary.

Signed-off-by: Andrew Banman <abanman@hpe.com>
Acked-by: Mike Travis <mike.travis@hpe.com>
---
 arch/x86/platform/uv/tlb_uv.c | 1 -
 1 file changed, 1 deletion(-)

Index: community/arch/x86/platform/uv/tlb_uv.c
===================================================================
--- community.orig/arch/x86/platform/uv/tlb_uv.c
+++ community/arch/x86/platform/uv/tlb_uv.c
@@ -1924,7 +1924,6 @@ static void pq_init(int node, int pnode)
 
 	ops.write_payload_first(pnode, first);
 	ops.write_payload_last(pnode, last);
-	ops.write_g_sw_ack(pnode, 0xffffUL);
 
 	/* in effect, all msg_type's are set to MSG_NOOP */
 	memset(pqp, 0, sizeof(struct bau_pq_entry) * DEST_Q_SIZE);

[toc] | [prev] | [next] | [standalone]

#1582826 — Re: [PATCH 5/6] x86/platform/uv/BAU: Remove initial write to swack register

On Tue, 14 Feb 2017, Andrew Banman wrote:

> Writing to the software acknowledge clear register when there are no
> pending messages causes a HUB error to assert. The original intent of this
> write was to clear the pending bits before start of operation, but this is
> an incorrect method and has been determined to be unnecessary.

This is a bug fix, independent of the rest of the series, right?

So it should go to the beginning of that series or even better sent out as
a seperate patch so it can be picked up right away. Hiding bug fixes in the
middle of a functional/feature series makes them slip through.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1580999 — [PATCH 1/6] x86/platform/uv/BAU: Declare bau_operations struct after other BAU structs

We must declare bau_operations after the other BAU structs so that we may
reference them in the bau_operations function declarations.

Signed-off-by: Andrew Banman <abanman@hpe.com>
Acked-by: Mike Travis <mike.travis@hpe.com>
---
 arch/x86/include/asm/uv/uv_bau.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/uv/uv_bau.h b/arch/x86/include/asm/uv/uv_bau.h
index 57ab86d..33234bc 100644
--- a/arch/x86/include/asm/uv/uv_bau.h
+++ b/arch/x86/include/asm/uv/uv_bau.h
@@ -385,17 +385,6 @@ struct uv2_3_bau_msg_header {
 	/* bits 127:120 */
 };
 
-/* Abstracted BAU functions */
-struct bau_operations {
-	unsigned long (*read_l_sw_ack)(void);
-	unsigned long (*read_g_sw_ack)(int pnode);
-	unsigned long (*bau_gpa_to_offset)(unsigned long vaddr);
-	void (*write_l_sw_ack)(unsigned long mmr);
-	void (*write_g_sw_ack)(int pnode, unsigned long mmr);
-	void (*write_payload_first)(int pnode, unsigned long mmr);
-	void (*write_payload_last)(int pnode, unsigned long mmr);
-};
-
 /*
  * The activation descriptor:
  * The format of the message to send, plus all accompanying control
@@ -644,6 +633,17 @@ struct bau_control {
 	struct hub_and_pnode	*thp;
 };
 
+/* Abstracted BAU functions */
+struct bau_operations {
+	unsigned long	(*read_l_sw_ack)(void);
+	unsigned long	(*read_g_sw_ack)(int pnode);
+	unsigned long	(*bau_gpa_to_offset)(unsigned long vaddr);
+	void		(*write_l_sw_ack)(unsigned long mmr);
+	void		(*write_g_sw_ack)(int pnode, unsigned long mmr);
+	void		(*write_payload_first)(int pnode, unsigned long mmr);
+	void		(*write_payload_last)(int pnode, unsigned long mmr);
+};
+
 static inline void write_mmr_data_broadcast(int pnode, unsigned long mmr_image)
 {
 	write_gmmr(pnode, UVH_BAU_DATA_BROADCAST, mmr_image);
-- 
1.8.2.1

[toc] | [prev] | [next] | [standalone]

#1582802 — Re: [PATCH 1/6] x86/platform/uv/BAU: Declare bau_operations struct after other BAU structs

On Tue, 14 Feb 2017, Andrew Banman wrote:

> We must declare bau_operations after the other BAU structs so that we may
> reference them in the bau_operations function declarations.

This changelog is misleading. I really had to look twice to figure out that
this is just a preparatory patch for adding stuff which references a bau
struct later. There is no 'MUST declare after'. You always can forward
declare structs when you only use a pointer in the struct op
declaration. Sure you can avoid it, but then please tell so, e.g.:

  Move the bau_operations declaration after bau struct declaration so the
  bau structs can be referenced when adding new functions to
  bau_operations. That way we avoid forward declarations of the bau
  structs.

Hmm?

>  
> +/* Abstracted BAU functions */
> +struct bau_operations {
> +	unsigned long	(*read_l_sw_ack)(void);
> +	unsigned long	(*read_g_sw_ack)(int pnode);
> +	unsigned long	(*bau_gpa_to_offset)(unsigned long vaddr);
> +	void		(*write_l_sw_ack)(unsigned long mmr);
> +	void		(*write_g_sw_ack)(int pnode, unsigned long mmr);
> +	void		(*write_payload_first)(int pnode, unsigned long mmr);
> +	void		(*write_payload_last)(int pnode, unsigned long mmr);

I appreciate that you made them tabular aligned!

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web