| Message ID | 1389714323-20130-8-git-send-email-eli@mellanox.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Le mardi 14 janvier 2014 à 17:45 +0200, Eli Cohen a écrit : > Put a wmb() to make sure the doorbell record is visible to the HCA before we > hit doorbell. > > Signed-off-by: Eli Cohen <eli@mellanox.com> > --- > drivers/infiniband/hw/mlx5/qp.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c > index a056c24..87b7fb1 100644 > --- a/drivers/infiniband/hw/mlx5/qp.c > +++ b/drivers/infiniband/hw/mlx5/qp.c > @@ -2251,6 +2251,10 @@ out: > > qp->db.db[MLX5_SND_DBR] = cpu_to_be32(qp->sq.cur_post); > > + /* Make sure doorbell record is visible to the HCA before > + * we hit doorbell */ > + wmb(); > + In linux-next, as of next-20140114, I'm seeing the write memory barrier before "qp->db.db[MLX5_SND_DBR] = cpu_to_be32(qp->sq.cur_post);" It was introduced by commit e126ba97dba9e: out: if (likely(nreq)) { qp->sq.head += nreq; /* Make sure that descriptors are written before * updating doorbell record and ringing the doorbell */ wmb(); qp->db.db[MLX5_SND_DBR] = cpu_to_be32(qp->sq.cur_post); if (bf->need_lock) spin_lock(&bf->lock); So this add a second wmb(). Is it really necessary ? > if (bf->need_lock) > spin_lock(&bf->lock); > Regards.
> > In linux-next, as of next-20140114, I'm seeing the write memory barrier > before "qp->db.db[MLX5_SND_DBR] = cpu_to_be32(qp->sq.cur_post);" > It was introduced by commit e126ba97dba9e: > > out: > if (likely(nreq)) { > qp->sq.head += nreq; > > /* Make sure that descriptors are written before > * updating doorbell record and ringing the doorbell > */ > wmb(); > > qp->db.db[MLX5_SND_DBR] = cpu_to_be32(qp->sq.cur_post); > > if (bf->need_lock) > spin_lock(&bf->lock); > > So this add a second wmb(). > Is it really necessary ? > Yes both are required. The first one makes sure that the descriptors are visible before update of the doorbell record, and the newly added one is required to ensure the doorbell record is visible before the hardware is notified. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le mercredi 15 janvier 2014 à 08:47 +0200, Eli Cohen a écrit : > > > > In linux-next, as of next-20140114, I'm seeing the write memory barrier > > before "qp->db.db[MLX5_SND_DBR] = cpu_to_be32(qp->sq.cur_post);" > > It was introduced by commit e126ba97dba9e: > > > > out: > > if (likely(nreq)) { > > qp->sq.head += nreq; > > > > /* Make sure that descriptors are written before > > * updating doorbell record and ringing the doorbell > > */ > > wmb(); > > > > qp->db.db[MLX5_SND_DBR] = cpu_to_be32(qp->sq.cur_post); > > > > if (bf->need_lock) > > spin_lock(&bf->lock); > > > > So this add a second wmb(). > > Is it really necessary ? > > > > Yes both are required. The first one makes sure that the descriptors > are visible before update of the doorbell record, and the newly added > one is required to ensure the doorbell record is visible before the > hardware is notified. OK. They are both needed to ensure ordered write (correct serialization), so that the descriptors are valids after reading the record. Regards.
diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c index a056c24..87b7fb1 100644 --- a/drivers/infiniband/hw/mlx5/qp.c +++ b/drivers/infiniband/hw/mlx5/qp.c @@ -2251,6 +2251,10 @@ out: qp->db.db[MLX5_SND_DBR] = cpu_to_be32(qp->sq.cur_post); + /* Make sure doorbell record is visible to the HCA before + * we hit doorbell */ + wmb(); + if (bf->need_lock) spin_lock(&bf->lock);
Put a wmb() to make sure the doorbell record is visible to the HCA before we hit doorbell. Signed-off-by: Eli Cohen <eli@mellanox.com> --- drivers/infiniband/hw/mlx5/qp.c | 4 ++++ 1 file changed, 4 insertions(+)