From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Wang Subject: Re: [2/5] vhost: enforce desc flags and content read ordering Date: Thu, 6 Dec 2018 12:24:48 +0800 Message-ID: References: <20181205094957.1938-3-maxime.coquelin@redhat.com> <16a4b2fd-c701-7822-3215-62e431e3f339@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: stable@dpdk.org, "Michael S. Tsirkin" To: Ilya Maximets , Maxime Coquelin , dev@dpdk.org, jfreimann@redhat.com, tiwei.bie@intel.com, zhihong.wang@intel.com Return-path: In-Reply-To: <16a4b2fd-c701-7822-3215-62e431e3f339@samsung.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2018/12/5 下午9:33, Ilya Maximets wrote: > On 05.12.2018 12:49, Maxime Coquelin wrote: >> A read barrier is required to ensure that the ordering between >> descriptor's flags and content reads is enforced. >> >> Fixes: 2f3225a7d69b ("vhost: add vector filling support for packed ring") >> Cc: stable@dpdk.org >> >> Reported-by: Jason Wang >> Signed-off-by: Maxime Coquelin >> --- >> lib/librte_vhost/virtio_net.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c >> index f11ebb54f..68b72e7a5 100644 >> --- a/lib/librte_vhost/virtio_net.c >> +++ b/lib/librte_vhost/virtio_net.c >> @@ -520,6 +520,12 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq, >> if (unlikely(!desc_is_avail(&descs[avail_idx], wrap_counter))) >> return -1; >> >> + /* >> + * The ordering between desc flags and desc >> + * content reads need to be enforced. >> + */ >> + rte_smp_rmb(); >> + > Same here. 'desc_is_avail' reads and uses the flags. i.e. > no way for reordering, > Writes must be ordered on the virtio side by the write barrier. > This means that if flags are updated (desc_is_avail() == true) > than the whole descriptor already updated and the data is written. > No need to have any read barriers here. In fact , the sequence might be: flag = read desc[avail_idx].flag [1] if(flag is not avail) {     read desc[avail_idx].id [2] } There's no data dependency but control dependency here, so 2 could be done before 1 without a rmb. Thanks > >> *desc_count = 0; >> *len = 0; >> >>