netdev - Re: [PATCH RFC]: napi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <adasl6vq8d1.fsf@cisco.com>
Date:	Tue, 07 Aug 2007 15:37:30 -0700
From:	Roland Dreier <rdreier@...co.com>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, shemminger@...ux-foundation.org,
	jgarzik@...ox.com, hadi@...erus.ca, rusty@...tcorp.com.au
Subject: Re: [PATCH RFC]: napi_struct V5

Thanks for looking at ipoib... overall looks fine, just a few comments.

 > --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
 > +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
 > @@ -281,63 +281,58 @@ static void ipoib_ib_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
 >  			   wc->status, wr_id, wc->vendor_err);
 >  }
 >  
 > -int ipoib_poll(struct net_device *dev, int *budget)
 > +int ipoib_poll(struct napi_struct *napi, int budget)

 > +poll_more:
 > +	while (done < budget) {
 > +		int max = (budget - done);
 > +
 >  		t = min(IPOIB_NUM_WC, max);

I think this is the only place where max is used now.  Might as well
kill it and put budget-done in directly.  That would get rid of the
strange-looking parens in the "max =" line too.

 >  		n = ib_poll_cq(priv->cq, t, priv->ibwc);
 >  
 > -		for (i = 0; i < n; ++i) {
 > +		for (i = 0; i < n; i++) {

it might be nicer to avoid noise like this in the patch.

 > +	if (done < budget) {
 > +		netif_rx_complete(dev, napi);
 >  		if (unlikely(ib_req_notify_cq(priv->cq,
 >  					      IB_CQ_NEXT_COMP |
 >  					      IB_CQ_REPORT_MISSED_EVENTS)) &&
 > -		    netif_rx_reschedule(dev, 0))
 > -			return 1;
 > -
 > -		return 0;
 > +		    netif_rx_reschedule(napi))
 > +			goto poll_more;

this goto back to the polling loop is a change in behavior.  When we
were tuning NAPI, we found that returning in the missed event case and
letting the NAPI core call the poll routine later actually performed
better, because it allowed more work to pile up.

So could the code just look like:

		netif_rx_complete(dev, napi);
 		if (unlikely(ib_req_notify_cq(priv->cq,
 					      IB_CQ_NEXT_COMP |
 					      IB_CQ_REPORT_MISSED_EVENTS)))
			netif_rx_reschedule(napi);

and then just return done in all cases?

It doesn't seem like the return value of netif_rx_reschedule() matters
in what we would want to do.  The only thing it's used for in the old
code is to decide what the poll routine should return.

 - R.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html