[openib-general] [PATCH][3/7]ipoib performance patches -- remove tx_ring

[Roland Dreier]

This also looks like a step backwards to me.  You are replacing a
cache-friendly array with a cache-unfriendly linked list, which also
requires two more lock/unlock operations in the fast path.

In a more general note, you are definitely generating some creative
ideas about IPoIB but you need to work on presentation and
communication if you want your patches integrated.  For example, all
you said about this patch is:

 > Here is the tx_ring removal patch for you to review. 

If you want this to be merged then you need to provide a changelog
entry that explains _what_ the patch does, _how_ it does it, and most
importantly _why_ the patch is an improvement.  This is especially
important for these patches, which naively at least look like they are
making things worse.

Also, improving the quality of your patches would be helpful.  For
example, in this patch you delete the comments:

 > -       /*
 > -        * We put the skb into the tx_ring _before_ we call post_send()
 > -        * because it's entirely possible that the completion handler will
 > -        * run before we execute anything after the post_send().  That
 > -        * means we have to make sure everything is properly recorded and
 > -        * our state is consistent before we call post_send().
 > -        */

and then a few lines further down you create exactly the bug it described:

 > +       err = post_send(priv, wr_id, address->ah, qpn, addr, skb->len);
 > +       if (!err) {
 > +               dev->trans_start = jiffies;
 > +               IPOIB_SKB_PRV_ADDR(skb) = addr;
 > +               IPOIB_SKB_PRV_AH(skb) = address;
 > +               IPOIB_SKB_PRV_SKB(skb) = skb;
 > +               spin_lock(&priv->slist_lock);
 > +               list_add_tail(&IPOIB_SKB_PRV_LIST(skb), &priv->send_list);
 > +               spin_unlock(&priv->slist_lock);
 > +               return;

This means I have to read your patches very carefully, because they
often introduce races or other bugs.

Finally, keeping strictly to the "one idea per patch" rule and holding
back from the temptation to fiddle with unrelated things would be
nice.  In this case you have:

 > -       spinlock_t           tx_lock    ____cacheline_aligned_in_smp;

 > +       spinlock_t           tx_lock;

That ____cacheline_aligned_in_smp annotation is something that you
added earlier in the patch series (with no explanation), and now
you're removing it (again with no explanation).  This sort of churn
just makes the patches bigger and harder to review and merge.

As I said you are generating very creative ideas for IPoIB.  Perhaps
you can find someone to help you polish the presentation so that we
can use them?

Thanks,
  Roland