[ofa-general] Re: [PATCH] IPoIB: Remove redundant check in xmit handler

[Krishna Kumar2]

Hi Roland,

I am not sure if my answer was clear, so I will try to be
clearer:

qdisc_run() first checks netif_queue_stopped(dev), and then if
it can get the __LINK_STATE_QDISC_RUNNING bit, it calls
__qdisc_run() which will do the actual xmit. Subsequent calls
to xmit within __qdisc_run checks for netif_queue_stopped.

So there is no way that xmit can be called with a stopped queue
as the core does it for every skb. And no other cpu can execute
this at the same time as the RUNNING bit is held. So this is a
completely safe removal of check for every skb.

I have tested this code extensively as part of batching skbs
and have never hit it.

Thanks,

- KK


> Hi Roland,
>
> > This check was added because of a real problem seen in practice a
> > while ago.  Has something changed in the tx queue locking that makes
> > it redundant now?
>
> I am not sure of how it was earlier, but currently a device's xmit can be
> called
> only on one cpu at a time (by holding the __LINK_STATE_QDISC_RUNNING
> bit in qdisc_run). And queue_stopped check is present before xmit.
>
> > I seem to remember that I could make the problem race trigger pretty
> > fast by making the tx queue very small so that it got stopped a lot.
>
> I just tested with a smaller queue size (tx queue size=4), put a debug in
> the
> queue_stopped check in xmit(), and a counter to find how many times the
> queue was stopped (in ipoib_send). After a 20 min test run with 64
threads,
> the queue was stopped 16.5 million times, but the debug never hit.
>
> I tested with buffer sizes varying from 128 to 16K bytes (though TSO/GSO
is
> not implemented in IPoIB anyway).
>
> Thanks,
>
> - KK
>
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit
http://openib.org/mailman/listinfo/openib-general