On Thu, Feb 13, 2020 at 08:10:12PM +0200, Leon Romanovsky wrote:
> On Thu, Feb 13, 2020 at 11:37:43AM -0400, Jason Gunthorpe wrote:
> > On Wed, Feb 12, 2020 at 09:26:30AM +0200, Leon Romanovsky wrote:
> >
> > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
> > > index 2aa3457a30ce..c614cb87d09b 100644
> > > +++ b/drivers/infiniband/ulp/ipoib/ipoib.h
> > > @@ -379,6 +379,7 @@ struct ipoib_dev_priv {
> > > struct ipoib_tx_buf *tx_ring;
> > > unsigned int tx_head;
> > > unsigned int tx_tail;
> > > + atomic_t tx_outstanding;
> > > struct ib_sge tx_sge[MAX_SKB_FRAGS + 1];
> > > struct ib_ud_wr tx_wr;
> > > struct ib_wc send_wc[MAX_SEND_CQE];
> > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> > > index c59e00a0881f..db6aace83fe5 100644
> > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> > > @@ -756,7 +756,7 @@ void ipoib_cm_send(struct net_device *dev, struct sk_buff *skb, struct ipoib_cm_
> > > return;
> > > }
> > >
> > > - if ((priv->tx_head - priv->tx_tail) == ipoib_sendq_size - 1) {
> > > + if (atomic_read(&priv->tx_outstanding) == ipoib_sendq_size - 1) {
> > > ipoib_dbg(priv, "TX ring 0x%x full, stopping kernel net queue\n",
> > > tx->qp->qp_num);
> > > netif_stop_queue(dev);
> > > @@ -786,7 +786,7 @@ void ipoib_cm_send(struct net_device *dev, struct sk_buff *skb, struct ipoib_cm_
> > > } else {
> > > netif_trans_update(dev);
> > > ++tx->tx_head;
> > > - ++priv->tx_head;
> > > + atomic_inc(&priv->tx_outstanding);
> > > }
> >
> > This use of an atomic is very weird, probably wrong.
> >
> > Why is it an atomic? priv->tx_head wasn't an atomic, and every place
> > touching tx_outstanding was also touching tx_head.
> >
> > I assume there is some hidden locking here? Or much more stuff is
> > busted up.
> >
> > In that case, drop the atomic.
> >
> > However, if the atomic is needed (where/why?) then something has to
> > be dealing with the races, and if the write side is fully locked then
> > an atomic is the wrong choice, use READ_ONCE/WRITE_ONCE instead
>
> I thought that atomic_t is appropriate here. Valentin wanted to
> implement "window" and he needed to ensure that this window is counted
> correctly while it doesn't need to be 100% accurate.
It does need to be 100% accurate because the stop_queue condition is:
+ if (atomic_read(&priv->tx_outstanding) == ipoib_sendq_size - 1) {
And presumably the whole problem here is overflowing some sized q
opbject.
That atomic is only needed if there is concurrency, and where is the
concurrency?
Jason
