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
Jason
