lkml.org 
[lkml]   [2012]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCHv3 1/3] NETPOLL: Extend rx_hook support.
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


\
 
 \ /
  Last update: 2012-02-28 00:37    [W:0.041 / U:3.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site