On Thu, Jul 12, 2007 at 03:54:32PM +0200, Olaf Kirch wrote:
> Hi Jarek,
>
> On Thursday 12 July 2007 14:59, Jarek Poplawski wrote:
>
> > > +#ifdef CONFIG_NETPOLL
> > > + /* Prevent race with netpoll - yes, this is a kludge.
> > > + * But at least it doesn't penalize the non-netpoll
> > > + * code path. */
> >
> > Alas, this can penalize those who have it enabled (e.g. by distro),
> > but don't use.
>
> Well, the test_bit is actually cheap; it's not atomic, and has no memory
> ordering requirements by all I know. The costly thing is set_bit/clear_bit
> in poll_napi; and you only ever get there when you *use* netpoll.
I've only meant "it doesn't penalize" isn't too precise here,
at least if you take it "mathematically". But, e.g. "politically"
it's 110% right, of course.
>
> > And it looks like _netif_rx_complete should be a better place,
> > at least considering such cards as: 8139too, skge, sungem and
> > maybe more (according to 2.6.22).
>
> Why?
It seems I miss something, but if it's to be called from dev->poll,
these drivers use __netif_rx_complete instead of netif_rx_complete.
>
> > > + set_bit(__LINK_STATE_POLL_LIST_FROZEN, &np->dev->state);
> > > npinfo->rx_flags |= NETPOLL_RX_DROP;
> >
> > I wonder, why this flag cannot be used for this check?
>
> I tried, but it made the patch rather icky. netpoll_info is defined
> in netpoll.h, which includes netdevice.h. So you cannot inline the
> check, and have to use an out-of-line function instead, along the
> lines of
>
> extern int am_i_being_called_by_poll_napi(struct net_device *);
>
> netif_rx_complete(struct net_device *dev)
> {
> #ifdef CONFIG_NETPOLL
> if (unlikely(dev->npinfo && am_i_being_called_by_poll_napi(dev))
> return;
> #endif
> ...
> }
>
> If you don't mind that, yes - this flag (or better, a newly introduced
> NETPOLL_RX_NAPI) may work as well.
>
> One thing I was a little worried about was whether dev->npinfo can
> go away all of a sudden. It's really just protected by an rcu_readlock...
I didn't think about this struct netpoll_info vs. inlining,
but I'm glad you did, so adding a new flag looks more reasonably
if we don't want to mess to much with netdevice.h.
BTW, I don't think there could be any problem with rcu (if it's
all about calling dev->poll from poll_napi) because then poll_napi
should have the same problem.
>
> > BTW, I'd be very glad if somebody could hint me what is the main
> > reason for such troublesome function as poll_napi: if it's about
> > performance isn't this enough to increase budget for netpoll in
> > net_rx_action?
>
> I think one reason is that you want to get the kernel oops out even
> when the machine is so hosed that it doesn't even service softirqs
> anymore.
Thanks! So, I have to think about this more. Of course, such idea
is fine if it doesn't collide with normal service, which I'm not
sure is true now. I mean this problem here, and e.g. needles
servicing of not netconsole packets by different cpus, but also
some unclear to me things like calling this from find_skb when
there is a problem with alloc_skb (I wonder how a card driver
manages to get these skbs for receiving?).
Cheers,
Jarek P.
-
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
