Re: [PATCH net-next 3/5] gianfar: GRO_DROP is unlikely

[Claudiu Manoil <claudiu.manoil@freescale.com>]

On 2/12/2013 6:30 PM, Paul Gortmaker wrote:
> [[PATCH net-next 3/5] gianfar: GRO_DROP is unlikely] On 12/02/2013 (Tue 14:47) Claudiu Manoil wrote:
>
>> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
>> ---
>>   drivers/net/ethernet/freescale/gianfar.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
>> index 096fb5f..5622134 100644
>> --- a/drivers/net/ethernet/freescale/gianfar.c
>> +++ b/drivers/net/ethernet/freescale/gianfar.c
>> @@ -2745,7 +2745,7 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
>>   	/* Send the packet up the stack */
>>   	ret = napi_gro_receive(napi, skb);
>>
>> -	if (GRO_DROP == ret)
>> +	if (unlikely(GRO_DROP == ret))
>>   		priv->extra_stats.kernel_dropped++;
>>
>>   	return 0;
>
> I wondered about this, specifically if it was a moot point, when the
> actual unlikely was deployed right at the end of the fcn.  It turns out
> that it does make a difference, since gfar_process_frame gets inlined,
> and so the increment code gets moved out of line (I have marked the if
> statment with * and the increment code within "-----"):
>
>   ------------------------- as is currently ------------------
>      4d14:       80 61 00 18     lwz     r3,24(r1)
>      4d18:       7f c4 f3 78     mr      r4,r30
>      4d1c:       48 00 00 01     bl      4d1c <gfar_clean_rx_ring+0x10c>
>   *  4d20:       2f 83 00 04     cmpwi   cr7,r3,4
>      4d24:       40 9e 00 1c     bne-    cr7,4d40 <gfar_clean_rx_ring+0x130>
> 		----------------------------
>      4d28:       81 3c 01 f8     lwz     r9,504(r28)
>      4d2c:       81 5c 01 fc     lwz     r10,508(r28)
>      4d30:       31 4a 00 01     addic   r10,r10,1
>      4d34:       7d 29 01 94     addze   r9,r9
>      4d38:       91 3c 01 f8     stw     r9,504(r28)
>      4d3c:       91 5c 01 fc     stw     r10,508(r28)
> 		----------------------------
>      4d40:       a0 1f 00 24     lhz     r0,36(r31)
>      4d44:       81 3f 00 00     lwz     r9,0(r31)
>      4d48:       7f a4 eb 78     mr      r4,r29
>      4d4c:       7f e3 fb 78     mr      r3,r31
>
>
>   -------------------------- unlikely ------------------------
>      4d14:       80 61 00 18     lwz     r3,24(r1)
>      4d18:       7f c4 f3 78     mr      r4,r30
>      4d1c:       48 00 00 01     bl      4d1c <gfar_clean_rx_ring+0x10c>
>   *  4d20:       2f 83 00 04     cmpwi   cr7,r3,4			
>      4d24:       41 9e 03 94     beq-    cr7,50b8 <gfar_clean_rx_ring+0x4a8>
>      4d28:       a0 1f 00 24     lhz     r0,36(r31)
>      4d2c:       81 3f 00 00     lwz     r9,0(r31)
>      4d30:       7f a4 eb 78     mr      r4,r29
>      4d34:       7f e3 fb 78     mr      r3,r31
> [...]
>      50b8:       81 3c 01 f8     lwz     r9,504(r28)
>      50bc:       81 5c 01 fc     lwz     r10,508(r28)
>      50c0:       31 4a 00 01     addic   r10,r10,1
>      50c4:       7d 29 01 94     addze   r9,r9
>      50c8:       91 3c 01 f8     stw     r9,504(r28)
>      50cc:       91 5c 01 fc     stw     r10,508(r28)
>      50d0:       4b ff fc 58     b       4d28 <gfar_clean_rx_ring+0x118>
>
> So, the increment does actually get moved ~1k away.  Maybe you can
> incorporate the above information in your long log, so the next guy
> doesn't wonder about the same question I did.
>
> Also, I noticed that gfar_process_frame() can be void instead of int.
> It never returns anything but zero, and the return code is ignored at
> the single call site.  Maybe you can add a patch to your series for that
> as well?
>
> Paul.
>
> .

Thanks for the notice.
The slightest code changes to gfar_process_frame() are reflected
to the driver's performance (i.e. throughput). So this is a very
"performance sensitive" function.
I'll see what happens if changed to return void.

Claudiu