On 05.12.2018 12:49, Maxime Coquelin wrote: > Cast to volatile is done when reading avail index and writing > the used index. This would not be necessary if proper barriers > are used.
'volatile' and barriers are not really connected. 'volatile' is the disabling of the compiler optimizations, while barriers are for runtime CPU level optimizations. In general, casts here made to force compiler to actually read the value and not cache it somehow. In fact that vhost library never writes to avail index, "very smart" compiler could drop it at all. None of modern compilers will do that for a single operation within a function, so, volatiles are not really necessary in current code, but they could save some nerves in case of code/compiler changes. OTOH, IMHO, the main purpose of the casts in current code is the self-documenting. Casts forces to pay special attention to these variables and reminds that they could be updated in other process. Casts allows to understand which variables are local and which are shared. I don't think that we should remove them anyway. > > Now that the read barrier has been added, we can remove these > cast to volatile. > > Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com> > --- > lib/librte_vhost/virtio_net.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c > index 679ce388b..eab1a5b4c 100644 > --- a/lib/librte_vhost/virtio_net.c > +++ b/lib/librte_vhost/virtio_net.c > @@ -114,7 +114,7 @@ flush_shadow_used_ring_split(struct virtio_net *dev, > struct vhost_virtqueue *vq) > > vhost_log_cache_sync(dev, vq); > > - *(volatile uint16_t *)&vq->used->idx += vq->shadow_used_idx; > + vq->used->idx += vq->shadow_used_idx; > vq->shadow_used_idx = 0; > vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx), > sizeof(vq->used->idx)); > @@ -794,7 +794,7 @@ virtio_dev_rx_split(struct virtio_net *dev, struct > vhost_virtqueue *vq, > struct buf_vector buf_vec[BUF_VECTOR_MAX]; > uint16_t avail_head; > > - avail_head = *((volatile uint16_t *)&vq->avail->idx); > + avail_head = vq->avail->idx; > > /* > * The ordering between avail index and > @@ -1379,8 +1379,7 @@ virtio_dev_tx_split(struct virtio_net *dev, struct > vhost_virtqueue *vq, > } > } > > - free_entries = *((volatile uint16_t *)&vq->avail->idx) - > - vq->last_avail_idx; > + free_entries = vq->avail->idx - vq->last_avail_idx; > if (free_entries == 0) > return 0; > > >
