[openib-general] [PATCH] IB/ipoib: NAPI

[eli at dev.mellanox.co.il]

>
> However I don't think we want to use peek CQ always -- I think that
> extra CQ lock/unlock may kill a lot of the performance gain you see
> with NAPI (and I don't think even mthca can do a lockless CQ peek,
> since we need to protect against races with resize CQ, etc).  So
> probably what we need is a feature bit in the struct ib_device to say
> whether the peek CQ is needed or whether req notify will generate
> events for existing CQEs.
>
Sounds good to me

> You might want to respin your patch against my for-2.6.19 branch -- I
> already split up the handle WC routine into separate send and receive
> functions, so the patch will become much smaller.
>

Sure. I can send an updated patch

> Also, the handling of how many completions to poll and the logic of
> when to call netif_rx_complete() seems very strange to me.  First, you
> totally ignore the budget parameter,
Not totally. I update it whenever I decide it is a "not_done" which
happens in two cases:
a. I finish my quota
b. I handled any completions - rx or tx. I do count tx as well since I
want to  coalesce as many completions as possible.

I do not update quota and budget when I exit polling mode because I think
there is no point in doing that (this is unlike the example in
NAPI-howto.txt but I will check again if this is the right thing to do).


>
> My poll routine ended up looking like the following, which I think is
> more correct:
>
> +int ipoib_poll(struct net_device *dev, int *budget)
> +{
> +	struct ipoib_dev_priv *priv = netdev_priv(dev);
> +	int max = min(*budget, dev->quota);
> +	int done = 0;
> +	int t;
> +	int empty = 0;
> +	int n, i;
> +
> +	while (max) {
> +		t = min(IPOIB_NUM_WC, max);
> +		n = ib_poll_cq(priv->cq, t, priv->ibwc);
> +
> +		for (i = 0; i < n; ++i) {
> +			if (priv->ibwc[i].wr_id & IPOIB_OP_RECV) {
> +				++done;
> +				--max;
> +				ipoib_ib_handle_rx_wc(dev, priv->ibwc + i);
> +			} else
> +				ipoib_ib_handle_tx_wc(dev, priv->ibwc + i);
> +		}
> +
> +		if (n != t) {
> +			empty = 1;
> +			break;
I don't think this is the right thing to do. Polling less completions then
you could is no reason to quit polling mode. You may receive more
completions in the next time you got called.

> +		}
> +	}
> +
> +	dev->quota -= done;
> +	*budget    -= done;
> +
> +	if (empty) {
> +		netif_rx_complete(dev);
> +		ib_req_notify_cq(priv->cq, IB_CQ_NEXT_COMP);
> +		/* XXX rotting packet! */
> +		return 0;
> +	}
> +
> +	return 1;
> +}
>