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
Julia Lawall <[email protected]> :
> 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
On Sun, 11 Aug 2013, Francois Romieu wrote:
> Julia Lawall <[email protected]> :
> > 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
Julia Lawall <[email protected]> :
> François Romieu <[email protected]> :
[...]
> > 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
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 <[email protected]> wrote:
> Julia Lawall <[email protected]> :
>> Fran?ois Romieu <[email protected]> :
> [...]
>> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
David Shwatrz <[email protected]> :
[...]
> 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
On Tue, 13 Aug 2013, Francois Romieu wrote:
> Julia Lawall <[email protected]> :
> > François Romieu <[email protected]> :
> [...]
> > > 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
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 <[email protected]> wrote:
> On Tue, 13 Aug 2013, Francois Romieu wrote:
>
>> Julia Lawall <[email protected]> :
>> > Fran?ois Romieu <[email protected]> :
>> [...]
>> > > 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
(no top-post nor lazy quote please)
David Shwatrz <[email protected]> :
[...]
> 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
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 <[email protected]> wrote:
> (no top-post nor lazy quote please)
>
> David Shwatrz <[email protected]> :
> [...]
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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