NAPI: netif_rx_reschedule() ??

NAPI: netif_rx_reschedule() ??

[Roland Dreier]

I'm looking at updating IP-over-InfiniBand to use NAPI, and due to the
way IB works, the driver is going to be susceptible to the rotting
packet problem.  It seems I'm going to have to call netif_rx_reschedule().

However I'm confused about a couple of things, and there are only two
uses of netif_rx_reschedule() in the kernel, so I'm a little stuck.

1. What is the intent of the second, 'undo' parameter?  For example,
   ibmveth.c does

	if(ibmveth_rxq_pending_buffer(adapter) && netif_rx_reschedule(netdev, frames_processed))
	{
		lpar_rc = h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_DISABLE);
		ibmveth_assert(lpar_rc == H_SUCCESS);
		more_work = 1;
		goto restart_poll;
	}

   but it only does

	netdev->quota -= frames_processed;

   _after_ that block (and the jump back to restart_poll).  So the
   whole things seems fishy: netdev->quota goes up by the number of
   frames processed??

2. What is the return value supposed to be used for?  ibmveth.c
   returns from the poll method if netif_rx_reschedule() returns 0 (ie
   the poll was already scheduled); ibm_emac_mal.c jumps back to
   repoll regardless of the return value, and NAPI_HOWTO.txt has

	if (ring_has_new_packet() && netif_rx_reschedule(dev, received)) {
		disable_rx_and_rxnobufs()
		goto restart_poll
	} while (rx_status_is_set);

   which is not particularly enlightening to say the least...
   if (...) {...} while (...) ?!

   It's not clear to me why the driver would want to do something
   different depending on whether the NAPI poll was already scheduled
   or not.

Thanks,
  Roland

Re: NAPI: netif_rx_reschedule() ??

[Stephen Hemminger]

On Wed, 30 Aug 2006 15:07:31 -0700
Roland Dreier <rdreier@cisco.com> wrote:

> I'm looking at updating IP-over-InfiniBand to use NAPI, and due to the
> way IB works, the driver is going to be susceptible to the rotting
> packet problem.  It seems I'm going to have to call netif_rx_reschedule().

Looking at it, I'm not sure netif_rx_reschedule was well thought out.
It shouldn't be inlined if many places start using it.

> However I'm confused about a couple of things, and there are only two
> uses of netif_rx_reschedule() in the kernel, so I'm a little stuck.
> 
> 1. What is the intent of the second, 'undo' parameter?  For example,
>    ibmveth.c does
> 
> 	if(ibmveth_rxq_pending_buffer(adapter) && netif_rx_reschedule(netdev, frames_processed))
> 	{
> 		lpar_rc = h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_DISABLE);
> 		ibmveth_assert(lpar_rc == H_SUCCESS);
> 		more_work = 1;
> 		goto restart_poll;
> 	}
> 
>    but it only does
> 
> 	netdev->quota -= frames_processed;
> 
>    _after_ that block (and the jump back to restart_poll).  So the
>    whole things seems fishy: netdev->quota goes up by the number of
>    frames processed??

The undo should really be handled by the caller, not in netif_rx_reschedule.
The existing interface assumes you have already deducted N from your quota
and so it needs to be put back.

> 
> 2. What is the return value supposed to be used for?  ibmveth.c
>    returns from the poll method if netif_rx_reschedule() returns 0 (ie
>    the poll was already scheduled); ibm_emac_mal.c jumps back to
>    repoll regardless of the return value, and NAPI_HOWTO.txt has
> 
> 	if (ring_has_new_packet() && netif_rx_reschedule(dev, received)) {
> 		disable_rx_and_rxnobufs()
> 		goto restart_poll
> 	} while (rx_status_is_set);
> 
>    which is not particularly enlightening to say the least...
>    if (...) {...} while (...) ?!
> 
>    It's not clear to me why the driver would want to do something
>    different depending on whether the NAPI poll was already scheduled
>    or not.

If poll is already scheduled, then after leaving this poll call,
another will occur.  If the poll routine just went ahead and rescanned
that would work as well. The important part is not to leave the
poll routine without being in one of these states:
	* all work is done, and hardware is empty and/or will interrupt for more.
	* some work was done, and device left on poll_list with softirq raised




-- 
Stephen Hemminger <shemminger@osdl.org>

Re: NAPI: netif_rx_reschedule() ??

[Roland Dreier]

    Stephen> The undo should really be handled by the caller, not in
    Stephen> netif_rx_reschedule.  The existing interface assumes you
    Stephen> have already deducted N from your quota and so it needs
    Stephen> to be put back.

makes sense -- especially since the caller probably also deducted N
from *budget, and netif_rx_reschedule() doesn't touch that.

    Stephen> If poll is already scheduled, then after leaving this
    Stephen> poll call, another will occur.  If the poll routine just
    Stephen> went ahead and rescanned that would work as well. The
    Stephen> important part is not to leave the poll routine without
    Stephen> being in one of these states: * all work is done, and
    Stephen> hardware is empty and/or will interrupt for more.  * some
    Stephen> work was done, and device left on poll_list with softirq
    Stephen> raised

But how does the return value of netif_rx_reschedule() help anything?
It either tells you that a poll was already scheduled, or that it
succeeded in scheduling a poll.  In either case another poll will
happen.  So why should the caller care?

Or is it possible that netif_rx_reschedule() returning 0 might mean
that it failed to reschedule the poll routine?  But even if that's
true, how can the caller use that -- 0 might mean the poll was already
rescheduled, and it might mean that the poll couldn't be resheduled,
but there's no way for the caller to know which.

 - R.

Re: NAPI: netif_rx_reschedule() ??

[Stephen Hemminger]

On Wed, 30 Aug 2006 16:31:12 -0700
Roland Dreier <rdreier@cisco.com> wrote:

>     Stephen> The undo should really be handled by the caller, not in
>     Stephen> netif_rx_reschedule.  The existing interface assumes you
>     Stephen> have already deducted N from your quota and so it needs
>     Stephen> to be put back.
> 
> makes sense -- especially since the caller probably also deducted N
> from *budget, and netif_rx_reschedule() doesn't touch that.

Changing it wouldn't be hard with only a couple of users.

> 
>     Stephen> If poll is already scheduled, then after leaving this
>     Stephen> poll call, another will occur.  If the poll routine just
>     Stephen> went ahead and rescanned that would work as well. The
>     Stephen> important part is not to leave the poll routine without
>     Stephen> being in one of these states: * all work is done, and
>     Stephen> hardware is empty and/or will interrupt for more.  * some
>     Stephen> work was done, and device left on poll_list with softirq
>     Stephen> raised
> 
> But how does the return value of netif_rx_reschedule() help anything?
> It either tells you that a poll was already scheduled, or that it
> succeeded in scheduling a poll.  In either case another poll will
> happen.  So why should the caller care?
> 
> Or is it possible that netif_rx_reschedule() returning 0 might mean
> that it failed to reschedule the poll routine?  But even if that's
> true, how can the caller use that -- 0 might mean the poll was already
> rescheduled, and it might mean that the poll couldn't be resheduled,
> but there's no way for the caller to know which.
> 
>  - R.

The return value doesn't really help. It should just be a void
function, the resulting state is the same in either case.

-- 
Stephen Hemminger <shemminger@osdl.org>

Re: NAPI: netif_rx_reschedule() ??

[Roland Dreier]

    Roland> makes sense -- especially since the caller probably also
    Roland> deducted N from *budget, and netif_rx_reschedule() doesn't
    Roland> touch that.

Actually, why does undoing the change to quota make sense?  Presumably
I passed N packets to netif_receive_skb() -- why shouldn't I deduct N
from quota and *budget?

 - R.

Re: NAPI: netif_rx_reschedule() ??

[Stephen Hemminger]

On Wed, 30 Aug 2006 16:39:14 -0700
Roland Dreier <rdreier@cisco.com> wrote:

>     Roland> makes sense -- especially since the caller probably also
>     Roland> deducted N from *budget, and netif_rx_reschedule() doesn't
>     Roland> touch that.
> 
> Actually, why does undoing the change to quota make sense?  Presumably
> I passed N packets to netif_receive_skb() -- why shouldn't I deduct N
> from quota and *budget?
> 
>  - R.

This was probably a preventive thing to avoid scheduling when quota remaining was zero?


-- 
Stephen Hemminger <shemminger@osdl.org>

Re: NAPI: netif_rx_reschedule() ??

[jamal]

On Thu, 2006-31-08 at 14:29 -0700, Stephen Hemminger wrote:
> On Wed, 30 Aug 2006 16:39:14 -0700
> Roland Dreier <rdreier@cisco.com> wrote:
> 
> >     Roland> makes sense -- especially since the caller probably also
> >     Roland> deducted N from *budget, and netif_rx_reschedule() doesn't
> >     Roland> touch that.
> > 
> > Actually, why does undoing the change to quota make sense?  Presumably
> > I passed N packets to netif_receive_skb() -- why shouldn't I deduct N
> > from quota and *budget?
> > 
> >  - R.
> 
> This was probably a preventive thing to avoid scheduling when quota remaining was zero?

I am reading backwards, so i may be incoherent ;-> 
I suppose if you dont trust the driver to tell you the truth it could be
done in the core code such as netif_receive_skb(). Should be noted
however, your quota does get refilled by the NAPI core governed by DRR
scheduling rules.

cheers,
jamal

Re: NAPI: netif_rx_reschedule() ??

[Alexey Kuznetsov]

Hello!

> However I'm confused about a couple of things, and there are only two
> uses of netif_rx_reschedule() in the kernel, so I'm a little stuck.

First, do not believe to even single bit of code or docs about
netif_rx_reschedule(). It was used once in the first version of NAPI
for 3com driver (which did not go to mainstream) and was left to rot. :-)


> 1. What is the intent of the second, 'undo' parameter?  For example,
>    ibmveth.c does
> 
> 	if(ibmveth_rxq_pending_buffer(adapter) && netif_rx_reschedule(netdev, frames_processed))
> 	{
> 		lpar_rc = h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_DISABLE);
> 		ibmveth_assert(lpar_rc == H_SUCCESS);
> 		more_work = 1;
> 		goto restart_poll;
> 	}
> 
>    but it only does
> 
> 	netdev->quota -= frames_processed;
> 
>    _after_ that block (and the jump back to restart_poll).  So the
>    whole things seems fishy: netdev->quota goes up by the number of
>    frames processed??

It is broken. netdev->quota MUST not be touched after netif_rx_complete().
Authors improved coding style, moving it closer to update of *budget
and it is wrong. First, because it is changed in an absolutely unserialized
context, second... you noticed.


>    It's not clear to me why the driver would want to do something
>    different depending on whether the NAPI poll was already scheduled
>    or not.

netif_rx_complete() released control. ->poll can be reentered on another
CPU after this.

If netif_rx_reschedule() fails, it means that ->poll must exit, because
poll was already rescheduled.

If it succeds, it means current thread got control back. Changes made
before netif_rx_complete(), becuase we were going to exit, (netdev->quota etc)
are undone and loop can be restarted.

To be honest, I do not recollect everything. F.e. scheduling softirq
in netif_rx_reschedule() looks like a cut-n-paste bug.

Alexey