Re: [Qemu-devel] [PATCH v5 3/6] libvhost-user: Support tracking inflight I/O in shared memory
On Fri, 1 Feb 2019 at 10:28, Jason Wang wrote:
>
>
> On 2019/1/30 下午1:48, Yongji Xie wrote:
> > On Wed, 30 Jan 2019 at 10:32, Jason Wang wrote:
> >>
> >> On 2019/1/22 下午4:31, elohi...@gmail.com wrote:
> >>> +static int
> >>> +vu_queue_inflight_get(VuDev *dev, VuVirtq *vq, int desc_idx)
> >>> +{
> >>> +if (!has_feature(dev->protocol_features,
> >>> +VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
> >>> +return 0;
> >>> +}
> >>> +
> >>> +if (unlikely(!vq->inflight)) {
> >>> +return -1;
> >>> +}
> >>> +
> >>> +vq->inflight->desc[desc_idx].inuse = 1;
> >>> +
> >>> +vq->inflight->desc[desc_idx].avail_idx = vq->last_avail_idx;
> >>> +
> >>> +return 0;
> >>> +}
> >>> +
> >>> +static int
> >>> +vu_queue_inflight_pre_put(VuDev *dev, VuVirtq *vq, int desc_idx)
> >>> +{
> >>> +if (!has_feature(dev->protocol_features,
> >>> +VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
> >>> +return 0;
> >>> +}
> >>> +
> >>> +if (unlikely(!vq->inflight)) {
> >>> +return -1;
> >>> +}
> >>> +
> >>> +vq->inflight->desc[desc_idx].used_idx = vq->used_idx;
> >>> +
> >>> +barrier();
> >>> +
> >>> +vq->inflight->desc[desc_idx].version++;
> >>> +
> >>> +return 0;
> >>> +}
> >>
> >> You probably need WRITE_ONCE() semantic (e.g volatile) to make sure the
> >> value reach memory.
> >>
> > The cache line should have been flushed during crash. So we can see
> > the correct value when backend reconnecting. If so, compile barrier
> > should be enough here, right?
>
>
> Maybe I worry too much but it's not about flushing cache, but about
> whether or not compiler can generate mov to memory instead of mov to
> registers.
>
OK, I see. I will declare those variables as volatile in v6. Thank you.
Thanks,
Yongji
Re: [Qemu-devel] [PATCH v5 3/6] libvhost-user: Support tracking inflight I/O in shared memory
On 2019/1/30 下午1:48, Yongji Xie wrote:
On Wed, 30 Jan 2019 at 10:32, Jason Wang wrote:
On 2019/1/22 下午4:31, elohi...@gmail.com wrote:
+static int
+vu_queue_inflight_get(VuDev *dev, VuVirtq *vq, int desc_idx)
+{
+if (!has_feature(dev->protocol_features,
+VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+return 0;
+}
+
+if (unlikely(!vq->inflight)) {
+return -1;
+}
+
+vq->inflight->desc[desc_idx].inuse = 1;
+
+vq->inflight->desc[desc_idx].avail_idx = vq->last_avail_idx;
+
+return 0;
+}
+
+static int
+vu_queue_inflight_pre_put(VuDev *dev, VuVirtq *vq, int desc_idx)
+{
+if (!has_feature(dev->protocol_features,
+VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+return 0;
+}
+
+if (unlikely(!vq->inflight)) {
+return -1;
+}
+
+vq->inflight->desc[desc_idx].used_idx = vq->used_idx;
+
+barrier();
+
+vq->inflight->desc[desc_idx].version++;
+
+return 0;
+}
You probably need WRITE_ONCE() semantic (e.g volatile) to make sure the
value reach memory.
The cache line should have been flushed during crash. So we can see
the correct value when backend reconnecting. If so, compile barrier
should be enough here, right?
Maybe I worry too much but it's not about flushing cache, but about
whether or not compiler can generate mov to memory instead of mov to
registers.
Thanks
Thanks,
Yongji
Re: [Qemu-devel] [PATCH v5 3/6] libvhost-user: Support tracking inflight I/O in shared memory
On 2019/1/30 上午11:58, Yongji Xie wrote:
On Wed, 30 Jan 2019 at 10:32, Jason Wang wrote:
On 2019/1/22 下午4:31, elohi...@gmail.com wrote:
+static int
+vu_queue_inflight_get(VuDev *dev, VuVirtq *vq, int desc_idx)
+{
+if (!has_feature(dev->protocol_features,
+VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+return 0;
+}
+
+if (unlikely(!vq->inflight)) {
+return -1;
+}
+
+vq->inflight->desc[desc_idx].inuse = 1;
+
+vq->inflight->desc[desc_idx].avail_idx = vq->last_avail_idx;
+
+return 0;
+}
+
+static int
+vu_queue_inflight_pre_put(VuDev *dev, VuVirtq *vq, int desc_idx)
+{
+if (!has_feature(dev->protocol_features,
+VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+return 0;
+}
+
+if (unlikely(!vq->inflight)) {
+return -1;
+}
+
+vq->inflight->desc[desc_idx].used_idx = vq->used_idx;
+
+barrier();
+
+vq->inflight->desc[desc_idx].version++;
+
+return 0;
+}
You probably need WRITE_ONCE() semantic (e.g volatile) to make sure the
value reach memory.
Is it enough to declare those variables as volatile?
Thanks,
Yongji
I think so.
Thanks
Re: [Qemu-devel] [PATCH v5 3/6] libvhost-user: Support tracking inflight I/O in shared memory
On 2019/1/30 上午11:14, Michael S. Tsirkin wrote:
On Wed, Jan 30, 2019 at 10:31:49AM +0800, Jason Wang wrote:
On 2019/1/22 下午4:31, elohi...@gmail.com wrote:
+static int
+vu_queue_inflight_get(VuDev *dev, VuVirtq *vq, int desc_idx)
+{
+if (!has_feature(dev->protocol_features,
+VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+return 0;
+}
+
+if (unlikely(!vq->inflight)) {
+return -1;
+}
+
+vq->inflight->desc[desc_idx].inuse = 1;
+
+vq->inflight->desc[desc_idx].avail_idx = vq->last_avail_idx;
+
+return 0;
+}
+
+static int
+vu_queue_inflight_pre_put(VuDev *dev, VuVirtq *vq, int desc_idx)
+{
+if (!has_feature(dev->protocol_features,
+VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+return 0;
+}
+
+if (unlikely(!vq->inflight)) {
+return -1;
+}
+
+vq->inflight->desc[desc_idx].used_idx = vq->used_idx;
+
+barrier();
+
+vq->inflight->desc[desc_idx].version++;
+
+return 0;
+}
You probably need WRITE_ONCE() semantic (e.g volatile) to make sure the
value reach memory.
Thanks
WRITE_ONCE is literally volatile + dependency memory barrier.
So unless compiler is a very agressive one, it does not
buy you much.
Well, since version is increased twice, if compiler decide the inline
both vu_queue_inflight_pre_put() and vu_queue_inflight_post_put(), can
we make sure it always generate instructions that write to memory
instead of registers?
Thanks
Re: [Qemu-devel] [PATCH v5 3/6] libvhost-user: Support tracking inflight I/O in shared memory
On Wed, 30 Jan 2019 at 10:32, Jason Wang wrote:
>
>
> On 2019/1/22 下午4:31, elohi...@gmail.com wrote:
> > +static int
> > +vu_queue_inflight_get(VuDev *dev, VuVirtq *vq, int desc_idx)
> > +{
> > +if (!has_feature(dev->protocol_features,
> > +VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
> > +return 0;
> > +}
> > +
> > +if (unlikely(!vq->inflight)) {
> > +return -1;
> > +}
> > +
> > +vq->inflight->desc[desc_idx].inuse = 1;
> > +
> > +vq->inflight->desc[desc_idx].avail_idx = vq->last_avail_idx;
> > +
> > +return 0;
> > +}
> > +
> > +static int
> > +vu_queue_inflight_pre_put(VuDev *dev, VuVirtq *vq, int desc_idx)
> > +{
> > +if (!has_feature(dev->protocol_features,
> > +VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
> > +return 0;
> > +}
> > +
> > +if (unlikely(!vq->inflight)) {
> > +return -1;
> > +}
> > +
> > +vq->inflight->desc[desc_idx].used_idx = vq->used_idx;
> > +
> > +barrier();
> > +
> > +vq->inflight->desc[desc_idx].version++;
> > +
> > +return 0;
> > +}
>
>
> You probably need WRITE_ONCE() semantic (e.g volatile) to make sure the
> value reach memory.
>
The cache line should have been flushed during crash. So we can see
the correct value when backend reconnecting. If so, compile barrier
should be enough here, right?
Thanks,
Yongji
Re: [Qemu-devel] [PATCH v5 3/6] libvhost-user: Support tracking inflight I/O in shared memory
On Wed, 30 Jan 2019 at 10:32, Jason Wang wrote:
>
>
> On 2019/1/22 下午4:31, elohi...@gmail.com wrote:
> > +static int
> > +vu_queue_inflight_get(VuDev *dev, VuVirtq *vq, int desc_idx)
> > +{
> > +if (!has_feature(dev->protocol_features,
> > +VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
> > +return 0;
> > +}
> > +
> > +if (unlikely(!vq->inflight)) {
> > +return -1;
> > +}
> > +
> > +vq->inflight->desc[desc_idx].inuse = 1;
> > +
> > +vq->inflight->desc[desc_idx].avail_idx = vq->last_avail_idx;
> > +
> > +return 0;
> > +}
> > +
> > +static int
> > +vu_queue_inflight_pre_put(VuDev *dev, VuVirtq *vq, int desc_idx)
> > +{
> > +if (!has_feature(dev->protocol_features,
> > +VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
> > +return 0;
> > +}
> > +
> > +if (unlikely(!vq->inflight)) {
> > +return -1;
> > +}
> > +
> > +vq->inflight->desc[desc_idx].used_idx = vq->used_idx;
> > +
> > +barrier();
> > +
> > +vq->inflight->desc[desc_idx].version++;
> > +
> > +return 0;
> > +}
>
>
> You probably need WRITE_ONCE() semantic (e.g volatile) to make sure the
> value reach memory.
>
Is it enough to declare those variables as volatile?
Thanks,
Yongji
Re: [Qemu-devel] [PATCH v5 3/6] libvhost-user: Support tracking inflight I/O in shared memory
On Wed, Jan 30, 2019 at 10:31:49AM +0800, Jason Wang wrote:
>
> On 2019/1/22 下午4:31, elohi...@gmail.com wrote:
> > +static int
> > +vu_queue_inflight_get(VuDev *dev, VuVirtq *vq, int desc_idx)
> > +{
> > +if (!has_feature(dev->protocol_features,
> > +VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
> > +return 0;
> > +}
> > +
> > +if (unlikely(!vq->inflight)) {
> > +return -1;
> > +}
> > +
> > +vq->inflight->desc[desc_idx].inuse = 1;
> > +
> > +vq->inflight->desc[desc_idx].avail_idx = vq->last_avail_idx;
> > +
> > +return 0;
> > +}
> > +
> > +static int
> > +vu_queue_inflight_pre_put(VuDev *dev, VuVirtq *vq, int desc_idx)
> > +{
> > +if (!has_feature(dev->protocol_features,
> > +VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
> > +return 0;
> > +}
> > +
> > +if (unlikely(!vq->inflight)) {
> > +return -1;
> > +}
> > +
> > +vq->inflight->desc[desc_idx].used_idx = vq->used_idx;
> > +
> > +barrier();
> > +
> > +vq->inflight->desc[desc_idx].version++;
> > +
> > +return 0;
> > +}
>
>
> You probably need WRITE_ONCE() semantic (e.g volatile) to make sure the
> value reach memory.
>
> Thanks
>
WRITE_ONCE is literally volatile + dependency memory barrier.
So unless compiler is a very agressive one, it does not
buy you much.
--
MST
Re: [Qemu-devel] [PATCH v5 3/6] libvhost-user: Support tracking inflight I/O in shared memory
On 2019/1/22 下午4:31, elohi...@gmail.com wrote:
+static int
+vu_queue_inflight_get(VuDev *dev, VuVirtq *vq, int desc_idx)
+{
+if (!has_feature(dev->protocol_features,
+VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+return 0;
+}
+
+if (unlikely(!vq->inflight)) {
+return -1;
+}
+
+vq->inflight->desc[desc_idx].inuse = 1;
+
+vq->inflight->desc[desc_idx].avail_idx = vq->last_avail_idx;
+
+return 0;
+}
+
+static int
+vu_queue_inflight_pre_put(VuDev *dev, VuVirtq *vq, int desc_idx)
+{
+if (!has_feature(dev->protocol_features,
+VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)) {
+return 0;
+}
+
+if (unlikely(!vq->inflight)) {
+return -1;
+}
+
+vq->inflight->desc[desc_idx].used_idx = vq->used_idx;
+
+barrier();
+
+vq->inflight->desc[desc_idx].version++;
+
+return 0;
+}
You probably need WRITE_ONCE() semantic (e.g volatile) to make sure the
value reach memory.
Thanks
