question about netif_rx

question about netif_rx

[Julia Lawall]

To my limited understanding, in a NAPI polling function, one should use 
netif_receive_skb, rather than netif_rx.  However, the via-velocity driver 
defines the NAPI polling function velocity_poll, which is the only caller 
of velocity_rx_srv, which is the only caller of velocity_receive_frame, 
which calls netif_rx.  The call to netif_rx seems to predate the 
introduction of NAPI in this driver.  Is this correct?

thanks,
julia

Re: question about netif_rx

[Francois Romieu]

Julia Lawall <julia.lawall@lip6.fr> :
> To my limited understanding, in a NAPI polling function, one should use 
> netif_receive_skb, rather than netif_rx.

Nit: or napi_gro_receive (+ napi_gro_flush with __napi_complete) when the
device offers some checksum offloading features.

> However, the via-velocity driver defines the NAPI polling function
> velocity_poll, which is the only caller of velocity_rx_srv, which
> is the only caller of velocity_receive_frame, which calls netif_rx.
> The call to netif_rx seems to predate the introduction of NAPI in
> this driver.  Is this correct?

You are right. It's a leftover of the NAPI changes in this driver.

Can you send a netif_receive_skb replacement patch for it ?

-- 
Ueimor

Re: question about netif_rx

[Julia Lawall]

On Sun, 11 Aug 2013, Francois Romieu wrote:

> Julia Lawall <julia.lawall@lip6.fr> :
> > To my limited understanding, in a NAPI polling function, one should use 
> > netif_receive_skb, rather than netif_rx.
> 
> Nit: or napi_gro_receive (+ napi_gro_flush with __napi_complete) when the
> device offers some checksum offloading features.

OK, thanks for the information.  I am just trying to understand these 
functions...

> > However, the via-velocity driver defines the NAPI polling function
> > velocity_poll, which is the only caller of velocity_rx_srv, which
> > is the only caller of velocity_receive_frame, which calls netif_rx.
> > The call to netif_rx seems to predate the introduction of NAPI in
> > this driver.  Is this correct?
> 
> You are right. It's a leftover of the NAPI changes in this driver.
> 
> Can you send a netif_receive_skb replacement patch for it ?

Just to be sure, I just replace netif_rx by netif_receive_skb, nothing
else?

thanks,
julia

Re: question about netif_rx

[Francois Romieu]

Julia Lawall <julia.lawall@lip6.fr> :
> François Romieu <romieu@fr.zoreil.com> :
[...]
> > Can you send a netif_receive_skb replacement patch for it ?
> 
> Just to be sure, I just replace netif_rx by netif_receive_skb, nothing
> else?

Yes. It should imho be fine with a comment incluing your analysis and a
few words about the current state of checksum offloading support.

-- 
Ueimor

Re: question about netif_rx

[David Shwatrz]

Hello,
what is the current state of checksum offloading support has to do
with it ? maybe you meant current state of NAPI support ?

regards,
DS

On Tue, Aug 13, 2013 at 8:20 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Julia Lawall <julia.lawall@lip6.fr> :
>> François Romieu <romieu@fr.zoreil.com> :
> [...]
>> > Can you send a netif_receive_skb replacement patch for it ?
>>
>> Just to be sure, I just replace netif_rx by netif_receive_skb, nothing
>> else?
>
> Yes. It should imho be fine with a comment incluing your analysis and a
> few words about the current state of checksum offloading support.
>
> --
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: question about netif_rx

[Francois Romieu]

David Shwatrz <dshwatrz@gmail.com> :
[...]
> what is the current state of checksum offloading support has to do
> with it ? maybe you meant current state of NAPI support ?

netif_receive_skb vs napi_gro_receive choice.

-- 
Ueimor

Re: question about netif_rx

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 565 bytes --]

On Tue, 13 Aug 2013, Francois Romieu wrote:

> Julia Lawall <julia.lawall@lip6.fr> :
> > François Romieu <romieu@fr.zoreil.com> :
> [...]
> > > Can you send a netif_receive_skb replacement patch for it ?
> > 
> > Just to be sure, I just replace netif_rx by netif_receive_skb, nothing
> > else?
> 
> Yes. It should imho be fine with a comment incluing your analysis and a
> few words about the current state of checksum offloading support.

I wouldn't know what to say about the checksum part.  It is not supported 
so I should use netif_receive_skb?

thanks,
julia

Re: question about netif_rx

[David Shwatrz]

Hello,
Sorry. I still don't understand what checksum has to do with it.
Does GRO depends on Rx/Tx checksum ? I don't think so.

In the napi_gro_receive() we check that the device supports
NETIF_F_GRO, but I don't see that we inspect checksum or that
NETIF_F_GRO is depends on checksum.

Can you please explain how checsum offload is related ?

rgs
David

On Tue, Aug 13, 2013 at 9:29 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Tue, 13 Aug 2013, Francois Romieu wrote:
>
>> Julia Lawall <julia.lawall@lip6.fr> :
>> > François Romieu <romieu@fr.zoreil.com> :
>> [...]
>> > > Can you send a netif_receive_skb replacement patch for it ?
>> >
>> > Just to be sure, I just replace netif_rx by netif_receive_skb, nothing
>> > else?
>>
>> Yes. It should imho be fine with a comment incluing your analysis and a
>> few words about the current state of checksum offloading support.
>
> I wouldn't know what to say about the checksum part.  It is not supported
> so I should use netif_receive_skb?
>
> thanks,
> julia

Re: question about netif_rx

[Francois Romieu]

(no top-post nor lazy quote please)

David Shwatrz <dshwatrz@gmail.com> :
[...]
> In the napi_gro_receive() we check that the device supports
> NETIF_F_GRO, but I don't see that we inspect checksum or that
> NETIF_F_GRO is depends on checksum.

napi_gro_receive is irrelevant. Let aside tunnel, the real work happens
in the protocol specific gro_receive handlers.

However I am an happy retard and I missed that tcp gro stopped depending
on Rx checksum since commit 861b650101eb0c627d171eb18de81dddb93d395e. :o/

So, yes, napi_gro_receive could be used.

-- 
Ueimor

Re: question about netif_rx

[Rami Rosen]

Hi,
BTW, this is not the only NAPI issue here. When looking into cleanup
of resources in this driver, a call to netif_napi_del() is missing
(though there is a call to  napi_disable(), which is not enough for
proper cleanup).

Best,
Rami Rosen
http://ramirose.wix.com/ramirosen

On Tue, Aug 13, 2013 at 11:41 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> (no top-post nor lazy quote please)
>
> David Shwatrz <dshwatrz@gmail.com> :
> [...]
>> In the napi_gro_receive() we check that the device supports
>> NETIF_F_GRO, but I don't see that we inspect checksum or that
>> NETIF_F_GRO is depends on checksum.
>
> napi_gro_receive is irrelevant. Let aside tunnel, the real work happens
> in the protocol specific gro_receive handlers.
>
> However I am an happy retard and I missed that tcp gro stopped depending
> on Rx checksum since commit 861b650101eb0c627d171eb18de81dddb93d395e. :o/
>
> So, yes, napi_gro_receive could be used.
>
> --
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: question about netif_rx

[Julia Lawall]

On Wed, 14 Aug 2013, Rami Rosen wrote:

> Hi,
> BTW, this is not the only NAPI issue here. When looking into cleanup
> of resources in this driver, a call to netif_napi_del() is missing
> (though there is a call to  napi_disable(), which is not enough for
> proper cleanup).

Actually, netif_napi_del seems to be quite underused.  At least
approximately, I would assume that every file that uses netif_napi_add
should use netif_napi_del at least once, and possible more times.  But I
find 118 .c files that use netif_napi_add, and only 35 that use
netif_napi_del.

Are there some cases where it might not be needed?

julia