Re: [PATCHv3 1/3] NETPOLL: Extend rx_hook support.

[Andrei Warkentin <awarkentin@vmware.com>]

Hi,

Thank you for the review, Jason. Comments inline.

----- Original Message -----
> From: "Jason Wessel" <jason.wessel@windriver.com>
> To: "Andrei Warkentin" <andrey.warkentin@gmail.com>
> Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, "Andrei Warkentin" <andreiw@vmware.com>,
> kgdb-bugreport@lists.sourceforge.net, "Matt Mackall" <mpm@selenic.com>
> Sent: Monday, February 27, 2012 6:17:12 PM
> Subject: Re: [PATCHv3 1/3] NETPOLL: Extend rx_hook support.
> 
> 
> I am just now starting to look how this patch set compares to kgdboe.
>  For the kgdboe the patch is a bit different.  The kgdboe opted to
> just pass the skb so as to cut down on the number of arguments to
> the function call.
> 
> From the kgdboe patch:
> 
> -       void (*rx_hook)(struct netpoll *, int, char *, int);
> +       void (*rx_hook)(struct netpoll *, int, char *, int, struct
> sk_buff *);
> 
> 

Interesting, I thought about passing the skb, but decided I didn't
want to copy and paste the skb parsing code, especially given
that it's always UDP anyway. I still have reservations about
passing the physical address, but I don't think anyone tried
to use netpoll or a non-ethernet device anyway.

> >  
> > +void netpoll_poll_dev(struct net_device *dev);
> >  void netpoll_send_udp(struct netpoll *np, const char *msg, int
> >  
> > -static void netpoll_poll_dev(struct net_device *dev)
> > +void netpoll_poll_dev(struct net_device *dev)
> 
> 
> This is interesting and I will have to look into this further...  A
> large part of the reason kgdboe never went anywhere was all around
> the locking problems the ability to safely use the network hardware
> and restore the state when it was done.  It appears you made this
> change so as to make a lockless call directly instead of going
> through netpoll_poll().  I am not entirely sure you could safely do
> this.
> 
> In kgdboe we always had:
> 
> +static int eth_get_char(void)
> +{
> +	int chr;
> +
> +	while (atomic_read(&in_count) == 0)
> +		netpoll_poll(&np);
> 
> 
> If it is the case that you really can safely call the
> netpoll_poll_dev() without the locks then the horrible sync irq
> state etc... could go away in kgdboe, and then it would be worth
> considering digging up all the ethernet polling errata fixes that
> live of out the mainline and perhaps submit some for review.
> 

I didn't look deeply at kgdboe (probably should have...). Anyway, netpoll_poll
doesn't seem to exist. netpoll_poll_dev is called from netpoll_send_skb_on_dev
and the only contract I see is running with the interrupts disabled - something
that is satisfied by running in the context of KDB.

This is slight OT, but...are WiFi drivers sufficiently similar that netpoll "just works?"

Thanks again.

A