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
> > --- a/drivers/infiniband/ulp/ipoib/ipoib.h
> > +++ 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
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> > +++ 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.
Thanks
>
> Jason
