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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 4 Mar 2019 06:45:26 -0800
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Xiaoyao Li <xiaoyao.li@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        x86 <x86@...nel.org>, kvm@...r.kernel.org
Subject: Re: [PATCH v4 02/17] drivers/net/b44: Align pwol_mask to unsigned
 long for better performance

On Mon, Mar 04, 2019 at 11:00:22AM +0100, Peter Zijlstra wrote:
> 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@...el.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;

Sure. I will change the patch to your code. And add
Signed-off-by: Peter Zijlstra <peterz@...radead.org>
to this patch?

Thanks.

-Fenghua