[openib-general] [PATCH][1/7]ipoib performance patches -- remove ah_reap

[Roland Dreier]

 > > But freeing AHs is something that happens infrequently and can be done
 > > asynchronously.  You're replacing that cost with two atomic operations
 > > per send packet!

 > No, actually it didn't free during sending during my test.

Sorry, I don't understand that answer at all.  It doesn't seem to be
responding to my point at all.  To reiterate: freeing AHs is a rare,
"slow path" operation that can be done asynchronously.  It is not a
good tradeoff to do two atomic_t operations for every sent packet,
just to avoid occasionally reaping AHs in process context.

 > > But can you guarantee that the AH stays around until after the send
 > > completes (which could be an arbitrarily long delay)?

 > I checked negih_add_path(), for unicast it is true always. See code below.
 > 
 > static void neigh_add_path(..)
 > {
 > ...
 >         if (path->ah) {
 >                 kref_get(&path->ah->ref);
 >                 neigh->ah = path->ah;
 >             ipoib_send(dev, skb, path->ah... 
 > } 

Again, I don't understand how this is a response at all.  The AH
cannot be freed until after the send operation is actually fully
completed, which could be a long time after ib_post_send() returns.

If an AH is freed after ipoib_send() returns but before the send is
executed, then the HCA may use stale data, which could lead to a send
error.

To summarize: the patch is broken (leads to incorrect lifetimes for
AHs), and in any case makes the send fast path slower.

 - R.