Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Dec 2020 08:58:46 +0200
From:   Eli Cohen <elic@nvidia.com>
To:     "Michael S. Tsirkin" <mst@redhat.com>
CC:     <jasowang@redhat.com>, <virtualization@lists.linux-foundation.org>,
        <linux-kernel@vger.kernel.org>, <lulu@redhat.com>
Subject: Re: [PATCH] vdpa/mlx5: Use write memory barrier after updating CQ
 index
Message-ID: <20201209065846.GA59515@mtl-vdi-166.wap.labs.mlnx>
References: <20201206105719.123753-1-elic@nvidia.com>
 <20201208164356-mutt-send-email-mst@kernel.org>
 <20201209060230.GA57362@mtl-vdi-166.wap.labs.mlnx>
 <20201209014547-mutt-send-email-mst@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20201209014547-mutt-send-email-mst@kernel.org>
User-Agent: Mutt/1.9.5 (bf161cf53efb) (2018-04-13)
Precedence: bulk

On Wed, Dec 09, 2020 at 01:46:22AM -0500, Michael S. Tsirkin wrote:
> On Wed, Dec 09, 2020 at 08:02:30AM +0200, Eli Cohen wrote:
> > On Tue, Dec 08, 2020 at 04:45:04PM -0500, Michael S. Tsirkin wrote:
> > > On Sun, Dec 06, 2020 at 12:57:19PM +0200, Eli Cohen wrote:
> > > > Make sure to put write memory barrier after updating CQ consumer index
> > > > so the hardware knows that there are available CQE slots in the queue.
> > > > 
> > > > Failure to do this can cause the update of the RX doorbell record to get
> > > > updated before the CQ consumer index resulting in CQ overrun.
> > > > 
> > > > Change-Id: Ib0ae4c118cce524c9f492b32569179f3c1f04cc1
> > > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > 
> > > Aren't both memory writes?
> > 
> > Not sure what exactly you mean here.
> 
> Both updates are CPU writes into RAM that hardware then reads
> using DMA.
> 

You mean why I did not put a memory barrier right after updating the
recieve doorbell record?

I thought about this and I think it is not required. Suppose it takes a
very long time till the hardware can actually see this update. The worst
effect would be that the hardware will drop received packets if it does
sees none available due to the delayed update. Eventually it will see
the update and will continue working.

If I put a memory barrier, I put some delay waiting for the CPU to flush
the write before continuing. I tried both options while checking packet
rate on couldn't see noticable difference in either case.

> > > And given that, isn't dma_wmb() sufficient here?
> > 
> > I agree that dma_wmb() is more appropriate here.
> > 
> > > 
> > > 
> > > > ---
> > > >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index 1f4089c6f9d7..295f46eea2a5 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -478,6 +478,11 @@ static int mlx5_vdpa_poll_one(struct mlx5_vdpa_cq *vcq)
> > > >  static void mlx5_vdpa_handle_completions(struct mlx5_vdpa_virtqueue *mvq, int num)
> > > >  {
> > > >  	mlx5_cq_set_ci(&mvq->cq.mcq);
> > > > +
> > > > +	/* make sure CQ cosumer update is visible to the hardware before updating
> > > > +	 * RX doorbell record.
> > > > +	 */
> > > > +	wmb();
> > > >  	rx_post(&mvq->vqqp, num);
> > > >  	if (mvq->event_cb.callback)
> > > >  		mvq->event_cb.callback(mvq->event_cb.private);
> > > > -- 
> > > > 2.27.0
> > > 
>