Re: [RFC] gro: Is it ok to share a single napi from several devs ?

[Stephen Hemminger <shemminger@vyatta.com>]

On Sat, 28 Aug 2010 16:48:29 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le samedi 28 août 2010 à 16:31 +0200, Jarek Poplawski a écrit :
> > On Sat, Aug 28, 2010 at 12:54:06PM +0200, Eric Dumazet wrote:
> > > In commit f2bde7328633269ee935d9ed96535ade15cc348f
> > > Author: Stephen Hemminger <shemminger@vyatta.com>
> > > 
> > >     net: allow multiple dev per napi with GRO
> > >     
> > >     GRO assumes that there is a one-to-one relationship between NAPI
> > >     structure and network device. Some devices like sky2 share multiple
> > >     devices on a single interrupt so only have one NAPI handler. Rather than
> > >     split GRO from NAPI, just have GRO assume if device changes that
> > >     it is a different flow.
> > >    
> > > 
> > > It was assumed a napi could be shared by several devs, but I dont really
> > > understand, since we have an unique ->dev pointer inside "napi_struct",
> > > this one is set once, and never change.
> > > 
> > > This pointer is currently used from napi_get_frags() [but that could be
> > > avoided], and from netpoll_poll_lock().
> > > 
> > > The netpoll_poll_lock() case is problematic.
> > > 
> > > static inline void *netpoll_poll_lock(struct napi_struct *napi)
> > > {
> > >         struct net_device *dev = napi->_dev;
> > > 
> > >         if (dev && dev->npinfo) {
> > >                 spin_lock(&napi->poll_lock);
> > > 
> > > Maybe we should remove 'dev' field from napi_struct and replace it by a
> > > npinfo pointer ?
> > 
> > Sky2 seems to work like a single netdev (with an internal sub-netdev),
> > so I can't see your concern: what is the main aim of this replacement?
> 
> I am trying to understand why this commit was needed then.
> 
> It adds an extra test in the main loop, testing skb->dev against p->dev,
> it must me for something...
> 
> I am trying to say that the one to one relationship between NAPI
> structure and a device is not only a GRO thing, but also a netpoll one.
> 
> So either we completely remove this one to one relationship, either
> Stephen commit was not needed.
> 
> Some clarification is needed.
> 
> 

The Marvell Yukon2 hardware supports two interfaces sharing a common interrupt.
Therfore the sky2 driver has up to two net devices and a single NAPI per board.
It is possible in a single invocation of the poll loop to process frames
for both ports.  GRO works by combining received packets from identical
flows over one NAPI interval. It is possible on sky2 that one packet
could be processed for the first port, and the next packet processed was for
second port and the two packets were related so that GRO would combine them.
The check for the same dev is required to prevent this. Yes it is an unlikely
corner case, but the purpose of GRO is to do aggregation but preserve the
flow characteristics of the incoming traffic.