[dpdk-dev] [PATCH v2 3/6] vhost: log used vring changes
On Tue, Dec 22, 2015 at 03:13:49PM +0800, Yuanhan Liu wrote:
> On Tue, Dec 22, 2015 at 02:55:52PM +0800, Peter Xu wrote:
> > On Thu, Dec 17, 2015 at 11:11:58AM +0800, Yuanhan Liu wrote:
> > > +static inline void __attribute__((always_inline))
> > > +vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
> > > + uint64_t offset, uint64_t len)
> > > +{
> >
> > One thing optional: I feel it a little bit confusing regarding to
> > the helper function name here, for the reasons:
> >
> > 1. It more sounds like "logging all the vrings we used", however,
> >what I understand is that, here we are logging dirty pages for
> >guest memory. Or say, there is merely nothing to do directly with
> >vring (although many vring ops might call this function, we are
> >only passing [buf, len] pairs).
> >
> > 2. It may also let people think of "vring_used", which is part of
> >virtio protocol. However, it does not mean it too.
>
> Yes, it does. Here log_guest_addr is set to physical address of
> vring_used. (check code at vhost_virtqueue_set_addr() of qemu).
>
> 627 static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
> 628 struct vhost_virtqueue *vq,
> 629 unsigned idx, bool enable_log)
> 630 {
> 631 struct vhost_vring_addr addr = {
> 632 .index = idx,
> 633 .desc_user_addr = (uint64_t)(unsigned long)vq->desc,
> 634 .avail_user_addr = (uint64_t)(unsigned long)vq->avail,
> 635 .used_user_addr = (uint64_t)(unsigned long)vq->used,
> ==> 636 .log_guest_addr = vq->used_phys,
> 637 .flags = enable_log ? (1 << VHOST_VRING_F_LOG) : 0,
> 638 };
>
> So, this function does log changes to used vring.
Yes. I have made a mistake.
Thanks!
Peter
[dpdk-dev] [PATCH v2 3/6] vhost: log used vring changes
On Tue, Dec 22, 2015 at 07:07:25AM +, Xie, Huawei wrote: > On 12/22/2015 2:56 PM, Peter Xu wrote: > > Got a question here: > > > > I see that we are logging down changes when we are marking > > used_vring. Do we need to log down buffer copy in rte_memcpy() too? > > I am not sure whether I understand it correctly, it seems that this > > is part of DPDK API ops to deliver data to the guest (from, e.g., > > OVS?), when we do rte_memcpy(), we seems to be modifying guest > > memory too. Am I wrong? > > > > Peter > > desc buffer logging isn't included in v1, but in the patch 4 of this > patch set, and actually it is the major work in vhost live migration. Yes, it is. Thanks to point out. Peter > --huawei > > [...] >
[dpdk-dev] [PATCH v2 3/6] vhost: log used vring changes
On Tue, Dec 22, 2015 at 02:55:52PM +0800, Peter Xu wrote:
> On Thu, Dec 17, 2015 at 11:11:58AM +0800, Yuanhan Liu wrote:
> > +static inline void __attribute__((always_inline))
> > +vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
> > +uint64_t offset, uint64_t len)
> > +{
>
> One thing optional: I feel it a little bit confusing regarding to
> the helper function name here, for the reasons:
>
> 1. It more sounds like "logging all the vrings we used", however,
>what I understand is that, here we are logging dirty pages for
>guest memory. Or say, there is merely nothing to do directly with
>vring (although many vring ops might call this function, we are
>only passing [buf, len] pairs).
>
> 2. It may also let people think of "vring_used", which is part of
>virtio protocol. However, it does not mean it too.
Yes, it does. Here log_guest_addr is set to physical address of
vring_used. (check code at vhost_virtqueue_set_addr() of qemu).
627 static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
628 struct vhost_virtqueue *vq,
629 unsigned idx, bool enable_log)
630 {
631 struct vhost_vring_addr addr = {
632 .index = idx,
633 .desc_user_addr = (uint64_t)(unsigned long)vq->desc,
634 .avail_user_addr = (uint64_t)(unsigned long)vq->avail,
635 .used_user_addr = (uint64_t)(unsigned long)vq->used,
==> 636 .log_guest_addr = vq->used_phys,
637 .flags = enable_log ? (1 << VHOST_VRING_F_LOG) : 0,
638 };
So, this function does log changes to used vring.
>
> I would suggest a better name without confusion, like
> vhost_log_dirty_range() or anything else to avoid those keywords.
>
> > + uint64_t addr;
> > +
> > + addr = vq->log_guest_addr + offset;
> > + vhost_log_write(dev, addr, len);
>
> Optional too: since addr is only used once, would it cleaner using
> one line? Like:
Yes, it is. Will fix it.
>
> vhost_log_write(dev, vq->log_guest_addr + offset, len);
>
> > +}
> > +
> > /**
> > * This function adds buffers to the virtio devices RX virtqueue. Buffers
> > can
> > * be received from the physical port or from another virtio device. A
> > packet
> > @@ -129,6 +139,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
> > uint32_t offset = 0, vb_offset = 0;
> > uint32_t pkt_len, len_to_cpy, data_len, total_copied = 0;
> > uint8_t hdr = 0, uncompleted_pkt = 0;
> > + uint16_t idx;
> >
> > /* Get descriptor from available ring */
> > desc = >desc[head[packet_success]];
> > @@ -200,16 +211,18 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t
> > queue_id,
> > }
> >
> > /* Update used ring with desc information */
> > - vq->used->ring[res_cur_idx & (vq->size - 1)].id =
> > - head[packet_success];
> > + idx = res_cur_idx & (vq->size - 1);
> > + vq->used->ring[idx].id = head[packet_success];
> >
> > /* Drop the packet if it is uncompleted */
> > if (unlikely(uncompleted_pkt == 1))
> > - vq->used->ring[res_cur_idx & (vq->size - 1)].len =
> > - vq->vhost_hlen;
> > + vq->used->ring[idx].len = vq->vhost_hlen;
> > else
> > - vq->used->ring[res_cur_idx & (vq->size - 1)].len =
> > - pkt_len +
> > vq->vhost_hlen;
> > + vq->used->ring[idx].len = pkt_len + vq->vhost_hlen;
> > +
> > + vhost_log_used_vring(dev, vq,
> > + offsetof(struct vring_used, ring[idx]),
> > + sizeof(vq->used->ring[idx]));
>
> Got a question here:
>
> I see that we are logging down changes when we are marking
> used_vring. Do we need to log down buffer copy in rte_memcpy() too?
> I am not sure whether I understand it correctly, it seems that this
> is part of DPDK API ops to deliver data to the guest (from, e.g.,
> OVS?), when we do rte_memcpy(), we seems to be modifying guest
> memory too. Am I wrong?
It's done in the next patch.
--yliu
[dpdk-dev] [PATCH v2 3/6] vhost: log used vring changes
On Thu, Dec 17, 2015 at 11:11:58AM +0800, Yuanhan Liu wrote:
> +static inline void __attribute__((always_inline))
> +vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
> + uint64_t offset, uint64_t len)
> +{
One thing optional: I feel it a little bit confusing regarding to
the helper function name here, for the reasons:
1. It more sounds like "logging all the vrings we used", however,
what I understand is that, here we are logging dirty pages for
guest memory. Or say, there is merely nothing to do directly with
vring (although many vring ops might call this function, we are
only passing [buf, len] pairs).
2. It may also let people think of "vring_used", which is part of
virtio protocol. However, it does not mean it too.
I would suggest a better name without confusion, like
vhost_log_dirty_range() or anything else to avoid those keywords.
> + uint64_t addr;
> +
> + addr = vq->log_guest_addr + offset;
> + vhost_log_write(dev, addr, len);
Optional too: since addr is only used once, would it cleaner using
one line? Like:
vhost_log_write(dev, vq->log_guest_addr + offset, len);
> +}
> +
> /**
> * This function adds buffers to the virtio devices RX virtqueue. Buffers can
> * be received from the physical port or from another virtio device. A packet
> @@ -129,6 +139,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
> uint32_t offset = 0, vb_offset = 0;
> uint32_t pkt_len, len_to_cpy, data_len, total_copied = 0;
> uint8_t hdr = 0, uncompleted_pkt = 0;
> + uint16_t idx;
>
> /* Get descriptor from available ring */
> desc = >desc[head[packet_success]];
> @@ -200,16 +211,18 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
> }
>
> /* Update used ring with desc information */
> - vq->used->ring[res_cur_idx & (vq->size - 1)].id =
> - head[packet_success];
> + idx = res_cur_idx & (vq->size - 1);
> + vq->used->ring[idx].id = head[packet_success];
>
> /* Drop the packet if it is uncompleted */
> if (unlikely(uncompleted_pkt == 1))
> - vq->used->ring[res_cur_idx & (vq->size - 1)].len =
> - vq->vhost_hlen;
> + vq->used->ring[idx].len = vq->vhost_hlen;
> else
> - vq->used->ring[res_cur_idx & (vq->size - 1)].len =
> - pkt_len +
> vq->vhost_hlen;
> + vq->used->ring[idx].len = pkt_len + vq->vhost_hlen;
> +
> + vhost_log_used_vring(dev, vq,
> + offsetof(struct vring_used, ring[idx]),
> + sizeof(vq->used->ring[idx]));
Got a question here:
I see that we are logging down changes when we are marking
used_vring. Do we need to log down buffer copy in rte_memcpy() too?
I am not sure whether I understand it correctly, it seems that this
is part of DPDK API ops to deliver data to the guest (from, e.g.,
OVS?), when we do rte_memcpy(), we seems to be modifying guest
memory too. Am I wrong?
Peter
>
> res_cur_idx++;
> packet_success++;
> @@ -236,6 +249,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>
> *(volatile uint16_t *)>used->idx += count;
> vq->last_used_idx = res_end_idx;
> + vhost_log_used_vring(dev, vq,
> + offsetof(struct vring_used, idx),
> + sizeof(vq->used->idx));
>
> /* flush used->idx update before we read avail->flags. */
> rte_mb();
> @@ -265,6 +281,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t
> queue_id,
> uint32_t seg_avail;
> uint32_t vb_avail;
> uint32_t cpy_len, entry_len;
> + uint16_t idx;
>
> if (pkt == NULL)
> return 0;
> @@ -302,16 +319,18 @@ copy_from_mbuf_to_vring(struct virtio_net *dev,
> uint32_t queue_id,
> entry_len = vq->vhost_hlen;
>
> if (vb_avail == 0) {
> - uint32_t desc_idx =
> - vq->buf_vec[vec_idx].desc_idx;
> + uint32_t desc_idx = vq->buf_vec[vec_idx].desc_idx;
> +
> + if ((vq->desc[desc_idx].flags & VRING_DESC_F_NEXT) == 0) {
> + idx = cur_idx & (vq->size - 1);
>
> - if ((vq->desc[desc_idx].flags
> - & VRING_DESC_F_NEXT) == 0) {
> /* Update used ring with desc information */
> - vq->used->ring[cur_idx & (vq->size - 1)].id
> - = vq->buf_vec[vec_idx].desc_idx;
> - vq->used->ring[cur_idx & (vq->size - 1)].len
> - = entry_len;
> + vq->used->ring[idx].id = vq->buf_vec[vec_idx].desc_idx;
> +
[dpdk-dev] [PATCH v2 3/6] vhost: log used vring changes
On 12/22/2015 2:56 PM, Peter Xu wrote:
> On Thu, Dec 17, 2015 at 11:11:58AM +0800, Yuanhan Liu wrote:
>> +static inline void __attribute__((always_inline))
>> +vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> + uint64_t offset, uint64_t len)
>> +{
[...]
> Got a question here:
>
> I see that we are logging down changes when we are marking
> used_vring. Do we need to log down buffer copy in rte_memcpy() too?
> I am not sure whether I understand it correctly, it seems that this
> is part of DPDK API ops to deliver data to the guest (from, e.g.,
> OVS?), when we do rte_memcpy(), we seems to be modifying guest
> memory too. Am I wrong?
>
> Peter
desc buffer logging isn't included in v1, but in the patch 4 of this
patch set, and actually it is the major work in vhost live migration.
--huawei
[...]
[dpdk-dev] [PATCH v2 3/6] vhost: log used vring changes
Introducing a vhost_log_write() wrapper, vhost_log_used_vring, to
log used vring changes.
Signed-off-by: Yuanhan Liu
Signed-off-by: Victor Kaplansky log_guest_addr + offset;
+ vhost_log_write(dev, addr, len);
+}
+
/**
* This function adds buffers to the virtio devices RX virtqueue. Buffers can
* be received from the physical port or from another virtio device. A packet
@@ -129,6 +139,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
uint32_t offset = 0, vb_offset = 0;
uint32_t pkt_len, len_to_cpy, data_len, total_copied = 0;
uint8_t hdr = 0, uncompleted_pkt = 0;
+ uint16_t idx;
/* Get descriptor from available ring */
desc = >desc[head[packet_success]];
@@ -200,16 +211,18 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
}
/* Update used ring with desc information */
- vq->used->ring[res_cur_idx & (vq->size - 1)].id =
- head[packet_success];
+ idx = res_cur_idx & (vq->size - 1);
+ vq->used->ring[idx].id = head[packet_success];
/* Drop the packet if it is uncompleted */
if (unlikely(uncompleted_pkt == 1))
- vq->used->ring[res_cur_idx & (vq->size - 1)].len =
- vq->vhost_hlen;
+ vq->used->ring[idx].len = vq->vhost_hlen;
else
- vq->used->ring[res_cur_idx & (vq->size - 1)].len =
- pkt_len +
vq->vhost_hlen;
+ vq->used->ring[idx].len = pkt_len + vq->vhost_hlen;
+
+ vhost_log_used_vring(dev, vq,
+ offsetof(struct vring_used, ring[idx]),
+ sizeof(vq->used->ring[idx]));
res_cur_idx++;
packet_success++;
@@ -236,6 +249,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
*(volatile uint16_t *)>used->idx += count;
vq->last_used_idx = res_end_idx;
+ vhost_log_used_vring(dev, vq,
+ offsetof(struct vring_used, idx),
+ sizeof(vq->used->idx));
/* flush used->idx update before we read avail->flags. */
rte_mb();
@@ -265,6 +281,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t
queue_id,
uint32_t seg_avail;
uint32_t vb_avail;
uint32_t cpy_len, entry_len;
+ uint16_t idx;
if (pkt == NULL)
return 0;
@@ -302,16 +319,18 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t
queue_id,
entry_len = vq->vhost_hlen;
if (vb_avail == 0) {
- uint32_t desc_idx =
- vq->buf_vec[vec_idx].desc_idx;
+ uint32_t desc_idx = vq->buf_vec[vec_idx].desc_idx;
+
+ if ((vq->desc[desc_idx].flags & VRING_DESC_F_NEXT) == 0) {
+ idx = cur_idx & (vq->size - 1);
- if ((vq->desc[desc_idx].flags
- & VRING_DESC_F_NEXT) == 0) {
/* Update used ring with desc information */
- vq->used->ring[cur_idx & (vq->size - 1)].id
- = vq->buf_vec[vec_idx].desc_idx;
- vq->used->ring[cur_idx & (vq->size - 1)].len
- = entry_len;
+ vq->used->ring[idx].id = vq->buf_vec[vec_idx].desc_idx;
+ vq->used->ring[idx].len = entry_len;
+
+ vhost_log_used_vring(dev, vq,
+ offsetof(struct vring_used, ring[idx]),
+ sizeof(vq->used->ring[idx]));
entry_len = 0;
cur_idx++;
@@ -354,10 +373,13 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t
queue_id,
if ((vq->desc[vq->buf_vec[vec_idx].desc_idx].flags &
VRING_DESC_F_NEXT) == 0) {
/* Update used ring with desc information */
- vq->used->ring[cur_idx & (vq->size - 1)].id
+ idx = cur_idx & (vq->size - 1);
+ vq->used->ring[idx].id
= vq->buf_vec[vec_idx].desc_idx;
- vq->used->ring[cur_idx & (vq->size - 1)].len
- = entry_len;
+ vq->used->ring[idx].len = entry_len;
+ vhost_log_used_vring(dev, vq,
+ offsetof(struct vring_used, ring[idx]),
+ sizeof(vq->used->ring[idx]));
entry_len = 0;
