Re: [PATCH v4 02/17] drivers/net/b44: Align pwol_mask to unsigned long for better performance

From: Peter Zijlstra <peterz@infradead.org>
To: Fenghua Yu <fenghua.yu@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	H Peter Anvin <hpa@zytor.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Ashok Raj <ashok.raj@intel.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>,
	Xiaoyao Li <xiaoyao.li@intel.com>,
	linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
	kvm@vger.kernel.org
Subject: Re: [PATCH v4 02/17] drivers/net/b44: Align pwol_mask to unsigned long for better performance
Date: Mon, 4 Mar 2019 11:00:22 +0100	[thread overview]
Message-ID: <20190304100022.GA32477@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <1551494711-213533-3-git-send-email-fenghua.yu@intel.com>

On Fri, Mar 01, 2019 at 06:44:56PM -0800, Fenghua Yu wrote:
> A bit in pwol_mask is set in b44_magic_pattern automatically by set_bit.
> set_bit sets the bit in a single unsigned long location. Since pwol_mask
> may not be aligned to unsigned long, the location may cross two cache
> lines and accessing the location degradates performance. On x86, accessing
> two cache lines in locked instruction in set_bit is called split lock and
> can cause overall performance degradation.
> 
> To avoid to impact performance by accessing two cache lines in set_bit,
> align pwol_mask to unsigned long.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  drivers/net/ethernet/broadcom/b44.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
> index 97ab0dd25552..bc544b6b9c3a 100644
> --- a/drivers/net/ethernet/broadcom/b44.c
> +++ b/drivers/net/ethernet/broadcom/b44.c
> @@ -1547,7 +1547,8 @@ static void b44_setup_pseudo_magicp(struct b44 *bp)
>  	u32 val;
>  	int plen0, plen1, plen2;
>  	u8 *pwol_pattern;
> -	u8 pwol_mask[B44_PMASK_SIZE];
> +	/* Align to unsigned long for better performance in set_bit() */
> +	u8 pwol_mask[B44_PMASK_SIZE] __aligned(sizeof(unsigned long));
>  
>  	pwol_pattern = kzalloc(B44_PATTERN_SIZE, GFP_KERNEL);
>  	if (!pwol_pattern)

That is truly horrid code. But afaict pwol_mask is local and never
exposed to concurrency, so _why_ does it need atomic bitset in the first
place?

Would not the below be a _much_ better solution?

diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 97ab0dd25552..0b4226b406b1 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -1520,7 +1520,7 @@ static int b44_magic_pattern(u8 *macaddr, u8 *ppattern, u8 *pmask, int offset)
 
 	memset(ppattern + offset, 0xff, magicsync);
 	for (j = 0; j < magicsync; j++)
-		set_bit(len++, (unsigned long *) pmask);
+		__set_bit(len++, (unsigned long *) pmask);
 
 	for (j = 0; j < B44_MAX_PATTERNS; j++) {
 		if ((B44_PATTERN_SIZE - len) >= ETH_ALEN)
@@ -1532,7 +1532,7 @@ static int b44_magic_pattern(u8 *macaddr, u8 *ppattern, u8 *pmask, int offset)
 		for (k = 0; k< ethaddr_bytes; k++) {
 			ppattern[offset + magicsync +
 				(j * ETH_ALEN) + k] = macaddr[k];
-			set_bit(len++, (unsigned long *) pmask);
+			__set_bit(len++, (unsigned long *) pmask);
 		}
 	}
 	return len - 1;
