This patch sets memory to zero directly to avoid unnecessary shift and
bitwise operations on bool type, which can fix a sparse warning and also
improve performance.
Signed-off-by: Rui Teng <[email protected]>
---
drivers/staging/wilc1000/host_interface.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 2568dfc15181..036c5c19a016 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -2416,10 +2416,9 @@ static void Handle_SetMulticastFilter(struct wilc_vif *vif,
goto ERRORHANDLER;
pu8CurrByte = wid.val;
- *pu8CurrByte++ = (strHostIfSetMulti->enabled & 0xFF);
- *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 8) & 0xFF);
- *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 16) & 0xFF);
- *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 24) & 0xFF);
+ memset(pu8CurrByte, 0, 4);
+ *pu8CurrByte = (strHostIfSetMulti->enabled & 0xFF);
+ pu8CurrByte += 4;
*pu8CurrByte++ = (strHostIfSetMulti->cnt & 0xFF);
*pu8CurrByte++ = ((strHostIfSetMulti->cnt >> 8) & 0xFF);
--
2.11.0
On Mon, Jul 10, 2017 at 04:57:31PM +0800, Rui Teng wrote:
> This patch sets memory to zero directly to avoid unnecessary shift and
> bitwise operations on bool type, which can fix a sparse warning and also
> improve performance.
It does? How did you measure the performance impact? What was now
faster?
thanks,
greg k-h
>
> Signed-off-by: Rui Teng <[email protected]>
> ---
> drivers/staging/wilc1000/host_interface.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index 2568dfc15181..036c5c19a016 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -2416,10 +2416,9 @@ static void Handle_SetMulticastFilter(struct wilc_vif *vif,
> goto ERRORHANDLER;
>
> pu8CurrByte = wid.val;
> - *pu8CurrByte++ = (strHostIfSetMulti->enabled & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 8) & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 16) & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 24) & 0xFF);
> + memset(pu8CurrByte, 0, 4);
> + *pu8CurrByte = (strHostIfSetMulti->enabled & 0xFF);
> + pu8CurrByte += 4;
Are you sure enabled isn't larger than 8 bits?
thanks,
greg k-h
On 12/07/2017 1:04 AM, Greg Kroah-Hartman wrote:
> On Mon, Jul 10, 2017 at 04:57:31PM +0800, Rui Teng wrote:
>> This patch sets memory to zero directly to avoid unnecessary shift and
>> bitwise operations on bool type, which can fix a sparse warning and also
>> improve performance.
>
> It does? How did you measure the performance impact? What was now
> faster?
It can avoid 3 times right shift and 3 times bitwise operations.
And once memory set should also faster than 4 times copy operations.
And add number 4 once should also faster than 4 times plus plus.
>
> thanks,
>
> greg k-h
>
>
>>
>> Signed-off-by: Rui Teng <[email protected]>
>> ---
>> drivers/staging/wilc1000/host_interface.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
>> index 2568dfc15181..036c5c19a016 100644
>> --- a/drivers/staging/wilc1000/host_interface.c
>> +++ b/drivers/staging/wilc1000/host_interface.c
>> @@ -2416,10 +2416,9 @@ static void Handle_SetMulticastFilter(struct wilc_vif *vif,
>> goto ERRORHANDLER;
>>
>> pu8CurrByte = wid.val;
>> - *pu8CurrByte++ = (strHostIfSetMulti->enabled & 0xFF);
>> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 8) & 0xFF);
>> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 16) & 0xFF);
>> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 24) & 0xFF);
>> + memset(pu8CurrByte, 0, 4);
>> + *pu8CurrByte = (strHostIfSetMulti->enabled & 0xFF);
>> + pu8CurrByte += 4;
>
> Are you sure enabled isn't larger than 8 bits?
The size of bool may larger than 8 bits. But when we assign any value
to bool type, the value of bool type will only be 1 or 0.
For example, the following output will be 1 other than 0x100.
bool b = 0x100;
printf("b: %d\n", b);
Or I think it should change 'enabled' type from bool to u32, to make
sure that the right shift operation can take effect.
>
> thanks,
>
> greg k-h
>
On Wed, Jul 12, 2017 at 10:23:02AM +0800, Rui Teng wrote:
> On 12/07/2017 1:04 AM, Greg Kroah-Hartman wrote:
> > On Mon, Jul 10, 2017 at 04:57:31PM +0800, Rui Teng wrote:
> > > This patch sets memory to zero directly to avoid unnecessary shift and
> > > bitwise operations on bool type, which can fix a sparse warning and also
> > > improve performance.
> >
> > It does? How did you measure the performance impact? What was now
> > faster?
>
> It can avoid 3 times right shift and 3 times bitwise operations.
> And once memory set should also faster than 4 times copy operations.
> And add number 4 once should also faster than 4 times plus plus.
And did you test this to prove that this does matter and is noticable?
How do you know that gcc doesn't just optimize it all away? Is this on
a code path that actually matters?
Don't ever say "improve performance" without actually being able to
prove it please.
> > thanks,
> >
> > greg k-h
> >
> >
> > >
> > > Signed-off-by: Rui Teng <[email protected]>
> > > ---
> > > drivers/staging/wilc1000/host_interface.c | 7 +++----
> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> > > index 2568dfc15181..036c5c19a016 100644
> > > --- a/drivers/staging/wilc1000/host_interface.c
> > > +++ b/drivers/staging/wilc1000/host_interface.c
> > > @@ -2416,10 +2416,9 @@ static void Handle_SetMulticastFilter(struct wilc_vif *vif,
> > > goto ERRORHANDLER;
> > >
> > > pu8CurrByte = wid.val;
> > > - *pu8CurrByte++ = (strHostIfSetMulti->enabled & 0xFF);
> > > - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 8) & 0xFF);
> > > - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 16) & 0xFF);
> > > - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 24) & 0xFF);
> > > + memset(pu8CurrByte, 0, 4);
> > > + *pu8CurrByte = (strHostIfSetMulti->enabled & 0xFF);
> > > + pu8CurrByte += 4;
> >
> > Are you sure enabled isn't larger than 8 bits?
>
> The size of bool may larger than 8 bits. But when we assign any value
> to bool type, the value of bool type will only be 1 or 0.
> For example, the following output will be 1 other than 0x100.
> bool b = 0x100;
> printf("b: %d\n", b);
>
> Or I think it should change 'enabled' type from bool to u32, to make sure
> that the right shift operation can take effect.
I think we should ask the authors of this code what is expected here as
it is quite odd...
thanks,
greg k-h
On Wed, 2017-07-12 at 08:12 +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 12, 2017 at 10:23:02AM +0800, Rui Teng wrote:
> > On 12/07/2017 1:04 AM, Greg Kroah-Hartman wrote:
> > > On Mon, Jul 10, 2017 at 04:57:31PM +0800, Rui Teng wrote:
> > > > This patch sets memory to zero directly to avoid unnecessary shift and
> > > > bitwise operations on bool type, which can fix a sparse warning and also
> > > > improve performance.
> > >
> > > It does? How did you measure the performance impact? What was now
> > > faster?
> >
> > It can avoid 3 times right shift and 3 times bitwise operations.
> > And once memory set should also faster than 4 times copy operations.
> > And add number 4 once should also faster than 4 times plus plus.
>
> And did you test this to prove that this does matter and is noticable?
> How do you know that gcc doesn't just optimize it all away? Is this on
> a code path that actually matters?
>
> Don't ever say "improve performance" without actually being able to
> prove it please.
Using __be32 would be more intelligible.
Maybe something like this: (in any number of patches)
o convert u8 *pu8CurrByte to be32 *buf
o convert strHostIfSetMulti to smulti
o remove useless initialization of result
o remove unnecessary parentheses
o return directly on kalloc failure
o remove label
Ending up with: (uncompiled/untested)
---
static void Handle_SetMulticastFilter(struct wilc_vif *vif,
??????struct set_multicast *smulti)
{
s32 result;
struct wid wid;
__be32 *buf;
wid.id = (u16)WID_SETUP_MULTICAST_FILTER;
wid.type = WID_BIN;
wid.size = sizeof(struct set_multicast) + smulti->cnt * ETH_ALEN;
wid.val = kmalloc(wid.size, GFP_KERNEL);
if (!wid.val)
return;
buf = (__force __be32 *)wid.val;
*buf++ = cpu_to_be32(smulti->enabled);
*buf++ = cpu_to_be32(smulti->cnt);
memcpy(buf, wilc_multicast_mac_addr_list, smulti->cnt * ETH_ALEN);
result = wilc_send_config_pkt(vif, SET_CFG, &wid, 1,
??????wilc_get_vif_idx(vif));
if (result)
netdev_err(vif->ndev, "Failed to send setup multicast\n");
kfree(wid.val);
}