| Message ID | 200911091647.29655.rusty@rustcorp.com.au |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Nov 9, 2009 at 8:17 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: >> > > +static void vhost_net_set_features(struct vhost_net *n, u64 features) >> > > +{ >> > > + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ? >> > > + sizeof(struct virtio_net_hdr) : 0; >> > > + int i; >> > > + mutex_lock(&n->dev.mutex); >> > > + n->dev.acked_features = features; >> > >> > Why is this called "acked_features"? Not just "features"? I expected >> > to see code which exposed these back to userspace, and didn't. >> >> Not sure how do you mean. Userspace sets them, why >> does it want to get them exposed back? > > There's something about the 'acked' which rubs me the wrong way. > "enabled_features" is perhaps a better term than "acked_features"; "acked" > seems more a user point-of-view, "enabled" seems more driver POV? > Hmm. Are you happy with the ioctl name? If yes I think being consistent with that is important. > set_features matches your ioctl names, but it sounds like a fn name :( > > It's marginal. And 'features' is shorter than both. I started with this but I was always getting confused whether this includes all features or just acked features. I'll go with enabled_features. > >> > > + switch (ioctl) { >> > > + case VHOST_SET_VRING_NUM: >> > >> > I haven't looked at your userspace implementation, but does a generic >> > VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more >> > sense? It'd be simpler here, >> >> Not by much though, right? >> >> > but not sure if it'd be simpler to use? >> >> The problem is with VHOST_SET_VRING_BASE as well. I want it to be >> separate because I want to make it possible to relocate e.g. used ring >> to another address while ring is running. This would be a good debugging >> tool (you look at kernel's used ring, check descriptor, then update >> guest's used ring) and also possibly an extra way to do migration. And >> it's nicer to have vring size separate as well, because it is >> initialized by host and never changed, right? > > Actually, this looks wrong to me: > > + case VHOST_SET_VRING_BASE: > ... > + vq->avail_idx = vq->last_avail_idx = s.num; > > The last_avail_idx is part of the state of the driver. It needs to be saved > and restored over susp/resume. Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a cached value for notify on empty, so what this does is clear the value. What exactly do you refer to when you say "this looks wrong"? This could trigger an extra notification if I ever called trigger_irq without get first. As I don't, it in fact has no effect. > The only reason it's not in the ring itself > is because I figured the other side doesn't need to see it (which is true, but > missed debugging opportunities as well as man-in-the-middle issues like this > one). I had a patch which put this field at the end of the ring, I might > resurrect it to avoid this problem. This is backwards compatible with all > implementations. See patch at end. Yes, I remember that patch. There seems to be little point though, at this stage. > > I would drop avail_idx altogether: get_user is basically free, and simplifies > a lot. As most state is in the ring, all you need is an ioctl to save/restore > the last_avail_idx. avail_idx is there for notify on empty: I had this thought that it's better to leave the avail cache line alone when we are triggering interrupt to avoid bouncing it around if guest is updating it meanwhile on another CPU, and I think my testing showed that it helped performance, but could be a mistake. You don't believe this can help? > >> We could merge DESC, AVAIL, USED, and it will reduce the amount of code >> in userspace. With both base, size and fds separate, it seemed a bit >> more symmetrical to have desc/avail/used separate as well. >> What's your opinion? > > Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT. Will do. > >> > For future reference, this is *exactly* the kind of thing which would have >> > been nice as a followup patch. Easy to separate, easy to review, not critical >> > to the core. >> >> Yes. It's not too late to split it out though: should I do it yet? > > Only if you're feeling enthused. It's lightly reviewed now. Not really :) I'll keep this in mind for the future. Thanks! > > Cheers, > Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 09, 2009 at 04:47:29PM +1030, Rusty Russell wrote: > On Sun, 8 Nov 2009 10:05:16 pm Michael S. Tsirkin wrote: > > On Fri, Nov 06, 2009 at 03:29:17PM +1030, Rusty Russell wrote: > > > > +/* Caller must have TX VQ lock */ > > > > +static void tx_poll_stop(struct vhost_net *net) > > > > +{ > > > > + if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED)) > > > > + return; > > > > > > likely? Really? > > > > Hmm ... yes. tx poll stop is called on each packet (as long as we do not > > fill up 1/2 backend queue), the first call will stop polling > > the rest checks state and does nothing. > > > > This is because we normally do not care when the message has left the > > queue in backend device: we tell backend to send it and forget. We only > > start polling when backend tx queue fills up. > > OK, good. > > > > > +static void vhost_net_set_features(struct vhost_net *n, u64 features) > > > > +{ > > > > + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ? > > > > + sizeof(struct virtio_net_hdr) : 0; > > > > + int i; > > > > + mutex_lock(&n->dev.mutex); > > > > + n->dev.acked_features = features; > > > > > > Why is this called "acked_features"? Not just "features"? I expected > > > to see code which exposed these back to userspace, and didn't. > > > > Not sure how do you mean. Userspace sets them, why > > does it want to get them exposed back? > > There's something about the 'acked' which rubs me the wrong way. > "enabled_features" is perhaps a better term than "acked_features"; "acked" > seems more a user point-of-view, "enabled" seems more driver POV? > > set_features matches your ioctl names, but it sounds like a fn name :( Hmm. Are you happy with the ioctl name? If yes I think being consistent with that is important. > It's marginal. And 'features' is shorter than both. I started with this but I was always getting confused whether this includes all features or just acked features. I'll go with enabled_features. > > > > + switch (ioctl) { > > > > + case VHOST_SET_VRING_NUM: > > > > > > I haven't looked at your userspace implementation, but does a generic > > > VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more > > > sense? It'd be simpler here, > > > > Not by much though, right? > > > > > but not sure if it'd be simpler to use? > > > > The problem is with VHOST_SET_VRING_BASE as well. I want it to be > > separate because I want to make it possible to relocate e.g. used ring > > to another address while ring is running. This would be a good debugging > > tool (you look at kernel's used ring, check descriptor, then update > > guest's used ring) and also possibly an extra way to do migration. And > > it's nicer to have vring size separate as well, because it is > > initialized by host and never changed, right? > > Actually, this looks wrong to me: > > + case VHOST_SET_VRING_BASE: > ... > + vq->avail_idx = vq->last_avail_idx = s.num; > > The last_avail_idx is part of the state of the driver. It needs to be saved > and restored over susp/resume. Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a cached value for notify on empty, so what this does is clear the value. What exactly do you refer to when you say "this looks wrong"? This could trigger an extra notification if I ever called trigger_irq without get first. As I don't, it in fact has no effect. > The only reason it's not in the ring itself > is because I figured the other side doesn't need to see it (which is true, but > missed debugging opportunities as well as man-in-the-middle issues like this > one). I had a patch which put this field at the end of the ring, I might > resurrect it to avoid this problem. This is backwards compatible with all > implementations. See patch at end. Yes, I remember that patch. There seems to be little point though, at this stage. > > I would drop avail_idx altogether: get_user is basically free, and > simplifies a lot. As most state is in the ring, all you need is an > ioctl to save/restore the last_avail_idx. avail_idx is there for notify on empty: I had this thought that it's better to leave the avail cache line alone when we are triggering interrupt to avoid bouncing it around if guest is updating it meanwhile on another CPU, and I think my testing showed that it helped performance, but could be a mistake. You don't believe this can help? > > We could merge DESC, AVAIL, USED, and it will reduce the amount of code > > in userspace. With both base, size and fds separate, it seemed a bit > > more symmetrical to have desc/avail/used separate as well. > > What's your opinion? > > Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT. OK, I'll do this. > > > For future reference, this is *exactly* the kind of thing which would have > > > been nice as a followup patch. Easy to separate, easy to review, not critical > > > to the core. > > > > Yes. It's not too late to split it out though: should I do it yet? > > Only if you're feeling enthused. It's lightly reviewed now. Not really :) I'll keep this in mind for the future. Thanks! > Cheers, > Rusty. > > virtio: put last_used and last_avail index into ring itself. > > Generally, the other end of the virtio ring doesn't need to see where > you're up to in consuming the ring. However, to completely understand > what's going on from the outside, this information must be exposed. > For example, if you want to save and restore a virtio_ring, but you're > not the consumer because the kernel is using it directly. > > Fortunately, we have room to expand: the ring is always a whole number > of pages and there's hundreds of bytes of padding after the avail ring > and the used ring, whatever the number of descriptors (which must be a > power of 2). > > We add a feature bit so the guest can tell the host that it's writing > out the current value there, if it wants to use that. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > --- > drivers/virtio/virtio_ring.c | 23 +++++++++++++++-------- > include/linux/virtio_ring.h | 12 +++++++++++- > 2 files changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -71,9 +71,6 @@ struct vring_virtqueue > /* Number we've added since last sync. */ > unsigned int num_added; > > - /* Last used index we've seen. */ > - u16 last_used_idx; > - > /* How to notify other side. FIXME: commonalize hcalls! */ > void (*notify)(struct virtqueue *vq); > > @@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt > > static inline bool more_used(const struct vring_virtqueue *vq) > { > - return vq->last_used_idx != vq->vring.used->idx; > + return vring_last_used(&vq->vring) != vq->vring.used->idx; > } > > static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len) > { > struct vring_virtqueue *vq = to_vvq(_vq); > + struct vring_used_elem *u; > void *ret; > unsigned int i; > > @@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu > return NULL; > } > > - i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id; > - *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len; > + u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num]; > + i = u->id; > + *len = u->len; > + /* Make sure we don't reload i after doing checks. */ > + rmb(); > > if (unlikely(i >= vq->vring.num)) { > BAD_RING(vq, "id %u out of range\n", i); > @@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu > /* detach_buf clears data, so grab it now. */ > ret = vq->data[i]; > detach_buf(vq, i); > - vq->last_used_idx++; > + vring_last_used(&vq->vring)++; > + > END_USE(vq); > return ret; > } > @@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un > vq->vq.name = name; > vq->notify = notify; > vq->broken = false; > - vq->last_used_idx = 0; > vq->num_added = 0; > list_add_tail(&vq->vq.list, &vdev->vqs); > #ifdef DEBUG > @@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un > > vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC); > > + /* We publish indices whether they offer it or not: if not, it's junk > + * space anyway. But calling this acknowledges the feature. */ > + virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES); > + > /* No callback? Tell other side not to bother us. */ > if (!callback) > vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT; > @@ -443,6 +448,8 @@ void vring_transport_features(struct vir > switch (i) { > case VIRTIO_RING_F_INDIRECT_DESC: > break; > + case VIRTIO_RING_F_PUBLISH_INDICES: > + break; > default: > /* We don't understand this bit. */ > clear_bit(i, vdev->features); > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > --- a/include/linux/virtio_ring.h > +++ b/include/linux/virtio_ring.h > @@ -29,6 +29,9 @@ > /* We support indirect buffer descriptors */ > #define VIRTIO_RING_F_INDIRECT_DESC 28 > > +/* We publish our last-seen used index at the end of the avail ring. */ > +#define VIRTIO_RING_F_PUBLISH_INDICES 29 > + > /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */ > struct vring_desc > { > @@ -87,6 +90,7 @@ struct vring { > * __u16 avail_flags; > * __u16 avail_idx; > * __u16 available[num]; > + * __u16 last_used_idx; > * > * // Padding to the next align boundary. > * char pad[]; > @@ -95,6 +99,7 @@ struct vring { > * __u16 used_flags; > * __u16 used_idx; > * struct vring_used_elem used[num]; > + * __u16 last_avail_idx; > * }; > */ > static inline void vring_init(struct vring *vr, unsigned int num, void *p, > @@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign > { > return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num) > + align - 1) & ~(align - 1)) > - + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num; > + + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2; > } > > +/* We publish the last-seen used index at the end of the available ring, and > + * vice-versa. These are at the end for backwards compatibility. */ > +#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num]) > +#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num]) > + > #ifdef __KERNEL__ > #include <linux/irqreturn.h> > struct virtio_device; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 09, 2009 at 04:47:29PM +1030, Rusty Russell wrote: > Actually, this looks wrong to me: > > + case VHOST_SET_VRING_BASE: > ... > + vq->avail_idx = vq->last_avail_idx = s.num; > > The last_avail_idx is part of the state of the driver. It needs to be saved > and restored over susp/resume. The only reason it's not in the ring itself > is because I figured the other side doesn't need to see it (which is true, but > missed debugging opportunities as well as man-in-the-middle issues like this > one). I had a patch which put this field at the end of the ring, I might > resurrect it to avoid this problem. This is backwards compatible with all > implementations. See patch at end. > > I would drop avail_idx altogether: get_user is basically free, and simplifies > a lot. As most state is in the ring, all you need is an ioctl to save/restore > the last_avail_idx. I remembered another reason for caching head in avail_idx. Basically, avail index could change between when I poll for descriptors and when I want to notify guest. So we could have: - poll descriptors until empty - notify detects not empty so does not notify And the way to solve it would be to return flag from notify telling us to restart the polling loop. But, this will be more code, on data path, than what happens today where I simply keep state from descriptor polling and use that to notify. I also suspect that somehow this race in practice can not create deadlocks ... but I prefer to avoid it, these things are very tricky: if I see an empty ring, and stop processing descriptors, I want to trigger notify on empty. So if we want to avoid keeping "empty" state, IMO the best way would be to pass a flag to vhost_signal that tells it that ring is empty. Makes sense?
On Mon, 9 Nov 2009 05:40:32 pm Michael S. Tsirkin wrote: > On Mon, Nov 9, 2009 at 8:17 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: > > There's something about the 'acked' which rubs me the wrong way. > > "enabled_features" is perhaps a better term than "acked_features"; "acked" > > seems more a user point-of-view, "enabled" seems more driver POV? > > Hmm. Are you happy with the ioctl name? If yes I think being consistent > with that is important. I think in my original comments I noted that I preferred GET / SET, rather than GET/ACK. > > Actually, this looks wrong to me: > > > > + case VHOST_SET_VRING_BASE: > > ... > > + vq->avail_idx = vq->last_avail_idx = s.num; > > > > The last_avail_idx is part of the state of the driver. It needs to be saved > > and restored over susp/resume. > > > Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a > cached value for notify on empty, so what this does is clear the value. Ah, you actually refresh it every time anyway. Hmm, could you do my poor brain a favor and either just get_user it in vhost_trigger_irq(), or call it 'cached_avail_idx' or something? > > The only reason it's not in the ring itself > > is because I figured the other side doesn't need to see it (which is true, but > > missed debugging opportunities as well as man-in-the-middle issues like this > > one). I had a patch which put this field at the end of the ring, I might > > resurrect it to avoid this problem. This is backwards compatible with all > > implementations. See patch at end. > > Yes, I remember that patch. There seems to be little point though, at > this stage. Well, it avoids this ioctl, by exposing all the state. We may well need it later, to expand the ring in other ways. > > I would drop avail_idx altogether: get_user is basically free, and simplifies > > a lot. As most state is in the ring, all you need is an ioctl to save/restore > > the last_avail_idx. > > avail_idx is there for notify on empty: I had this thought that it's > better to leave the avail cache line alone when we are triggering > interrupt to avoid bouncing it around if guest is updating it meanwhile > on another CPU, and I think my testing showed that it helped > performance, but could be a mistake. You don't believe this can help? I believe it could help, but this is YA case where it would have been nice to have a dumb basic patch and this as a patch on top. But I am going to ask you to re-run that measurement, see if it stacks up (because it's an interesting lesson if it does..) Thanks! Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> virtio: put last_used and last_avail index into ring itself. > > Generally, the other end of the virtio ring doesn't need to see where > you're up to in consuming the ring. However, to completely understand > what's going on from the outside, this information must be exposed. > For example, if you want to save and restore a virtio_ring, but you're > not the consumer because the kernel is using it directly. > > Fortunately, we have room to expand: the ring is always a whole number > of pages and there's hundreds of bytes of padding after the avail ring > and the used ring, whatever the number of descriptors (which must be a > power of 2). > > We add a feature bit so the guest can tell the host that it's writing > out the current value there, if it wants to use that. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> I've been looking at this patch some more (more on why later), and I wonder: would it be better to add some alignment to the last used index address, so that if we later add more stuff at the tail, it all fits in a single cache line? We use a new feature bit anyway, so layout change should not be a problem. Since I raised the question of caches: for used ring, the ring is not aligned to 64 bit, so on CPUs with 64 bit or larger cache lines, used entries will often cross cache line boundaries. Am I right and might it have been better to align ring entries to cache line boundaries? What do you think? > --- > drivers/virtio/virtio_ring.c | 23 +++++++++++++++-------- > include/linux/virtio_ring.h | 12 +++++++++++- > 2 files changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -71,9 +71,6 @@ struct vring_virtqueue > /* Number we've added since last sync. */ > unsigned int num_added; > > - /* Last used index we've seen. */ > - u16 last_used_idx; > - > /* How to notify other side. FIXME: commonalize hcalls! */ > void (*notify)(struct virtqueue *vq); > > @@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt > > static inline bool more_used(const struct vring_virtqueue *vq) > { > - return vq->last_used_idx != vq->vring.used->idx; > + return vring_last_used(&vq->vring) != vq->vring.used->idx; > } > > static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len) > { > struct vring_virtqueue *vq = to_vvq(_vq); > + struct vring_used_elem *u; > void *ret; > unsigned int i; > > @@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu > return NULL; > } > > - i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id; > - *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len; > + u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num]; > + i = u->id; > + *len = u->len; > + /* Make sure we don't reload i after doing checks. */ > + rmb(); > > if (unlikely(i >= vq->vring.num)) { > BAD_RING(vq, "id %u out of range\n", i); > @@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu > /* detach_buf clears data, so grab it now. */ > ret = vq->data[i]; > detach_buf(vq, i); > - vq->last_used_idx++; > + vring_last_used(&vq->vring)++; > + > END_USE(vq); > return ret; > } > @@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un > vq->vq.name = name; > vq->notify = notify; > vq->broken = false; > - vq->last_used_idx = 0; > vq->num_added = 0; > list_add_tail(&vq->vq.list, &vdev->vqs); > #ifdef DEBUG > @@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un > > vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC); > > + /* We publish indices whether they offer it or not: if not, it's junk > + * space anyway. But calling this acknowledges the feature. */ > + virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES); > + > /* No callback? Tell other side not to bother us. */ > if (!callback) > vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT; > @@ -443,6 +448,8 @@ void vring_transport_features(struct vir > switch (i) { > case VIRTIO_RING_F_INDIRECT_DESC: > break; > + case VIRTIO_RING_F_PUBLISH_INDICES: > + break; > default: > /* We don't understand this bit. */ > clear_bit(i, vdev->features); > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > --- a/include/linux/virtio_ring.h > +++ b/include/linux/virtio_ring.h > @@ -29,6 +29,9 @@ > /* We support indirect buffer descriptors */ > #define VIRTIO_RING_F_INDIRECT_DESC 28 > > +/* We publish our last-seen used index at the end of the avail ring. */ > +#define VIRTIO_RING_F_PUBLISH_INDICES 29 > + > /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */ > struct vring_desc > { > @@ -87,6 +90,7 @@ struct vring { > * __u16 avail_flags; > * __u16 avail_idx; > * __u16 available[num]; > + * __u16 last_used_idx; > * > * // Padding to the next align boundary. > * char pad[]; > @@ -95,6 +99,7 @@ struct vring { > * __u16 used_flags; > * __u16 used_idx; > * struct vring_used_elem used[num]; > + * __u16 last_avail_idx; > * }; > */ > static inline void vring_init(struct vring *vr, unsigned int num, void *p, > @@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign > { > return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num) > + align - 1) & ~(align - 1)) > - + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num; > + + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2; > } > > +/* We publish the last-seen used index at the end of the available ring, and > + * vice-versa. These are at the end for backwards compatibility. */ > +#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num]) > +#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num]) > + > #ifdef __KERNEL__ > #include <linux/irqreturn.h> > struct virtio_device; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote: > > virtio: put last_used and last_avail index into ring itself. > > > > Generally, the other end of the virtio ring doesn't need to see where > > you're up to in consuming the ring. However, to completely understand > > what's going on from the outside, this information must be exposed. > > For example, if you want to save and restore a virtio_ring, but you're > > not the consumer because the kernel is using it directly. > > > > Fortunately, we have room to expand: the ring is always a whole number > > of pages and there's hundreds of bytes of padding after the avail ring > > and the used ring, whatever the number of descriptors (which must be a > > power of 2). > > > > We add a feature bit so the guest can tell the host that it's writing > > out the current value there, if it wants to use that. > > > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > I've been looking at this patch some more (more on why > later), and I wonder: would it be better to add some > alignment to the last used index address, so that > if we later add more stuff at the tail, it all > fits in a single cache line? In theory, but not in practice. We don't have many rings, so the difference between 1 and 2 cache lines is not very much. > We use a new feature bit anyway, so layout change should not be > a problem. > > Since I raised the question of caches: for used ring, > the ring is not aligned to 64 bit, so on CPUs with 64 bit > or larger cache lines, used entries will often cross > cache line boundaries. Am I right and might it > have been better to align ring entries to cache line boundaries? > > What do you think? I think everyone is settled on 128 byte cache lines for the forseeable future, so it's not really an issue. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 06, 2010 at 10:22:12AM +0930, Rusty Russell wrote: > On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote: > > > virtio: put last_used and last_avail index into ring itself. > > > > > > Generally, the other end of the virtio ring doesn't need to see where > > > you're up to in consuming the ring. However, to completely understand > > > what's going on from the outside, this information must be exposed. > > > For example, if you want to save and restore a virtio_ring, but you're > > > not the consumer because the kernel is using it directly. > > > > > > Fortunately, we have room to expand: the ring is always a whole number > > > of pages and there's hundreds of bytes of padding after the avail ring > > > and the used ring, whatever the number of descriptors (which must be a > > > power of 2). > > > > > > We add a feature bit so the guest can tell the host that it's writing > > > out the current value there, if it wants to use that. > > > > > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > > > I've been looking at this patch some more (more on why > > later), and I wonder: would it be better to add some > > alignment to the last used index address, so that > > if we later add more stuff at the tail, it all > > fits in a single cache line? > > In theory, but not in practice. We don't have many rings, so the > difference between 1 and 2 cache lines is not very much. Fair enough. > > We use a new feature bit anyway, so layout change should not be > > a problem. > > > > Since I raised the question of caches: for used ring, > > the ring is not aligned to 64 bit, so on CPUs with 64 bit > > or larger cache lines, used entries will often cross > > cache line boundaries. Am I right and might it > > have been better to align ring entries to cache line boundaries? > > > > What do you think? > > I think everyone is settled on 128 byte cache lines for the forseeable > future, so it's not really an issue. > > Cheers, > Rusty. You mean with 64 bit descriptors we will be bouncing a cache line between host and guest, anyway?
On Thu, 6 May 2010 03:57:55 pm Michael S. Tsirkin wrote: > On Thu, May 06, 2010 at 10:22:12AM +0930, Rusty Russell wrote: > > On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote: > > > What do you think? > > > > I think everyone is settled on 128 byte cache lines for the forseeable > > future, so it's not really an issue. > > You mean with 64 bit descriptors we will be bouncing a cache line > between host and guest, anyway? I'm confused by this entire thread. Descriptors are 16 bytes. They are at the start, so presumably aligned to cache boundaries. Available ring follows that at 2 bytes per entry, so it's also packed nicely into cachelines. Then there's padding to page boundary. That puts us on a cacheline again for the used ring; also 2 bytes per entry. I don't see how any change in layout could be more cache friendly? Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 07, 2010 at 12:35:39PM +0930, Rusty Russell wrote: > On Thu, 6 May 2010 03:57:55 pm Michael S. Tsirkin wrote: > > On Thu, May 06, 2010 at 10:22:12AM +0930, Rusty Russell wrote: > > > On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote: > > > > What do you think? > > > > > > I think everyone is settled on 128 byte cache lines for the forseeable > > > future, so it's not really an issue. > > > > You mean with 64 bit descriptors we will be bouncing a cache line > > between host and guest, anyway? > > I'm confused by this entire thread. > > Descriptors are 16 bytes. They are at the start, so presumably aligned to > cache boundaries. > > Available ring follows that at 2 bytes per entry, so it's also packed nicely > into cachelines. > > Then there's padding to page boundary. That puts us on a cacheline again > for the used ring; also 2 bytes per entry. > Hmm, is used ring really 2 bytes per entry? /* u32 is used here for ids for padding reasons. */ struct vring_used_elem { /* Index of start of used descriptor chain. */ __u32 id; /* Total length of the descriptor chain which was used (written to) */ __u32 len; }; struct vring_used { __u16 flags; __u16 idx; struct vring_used_elem ring[]; }; > I don't see how any change in layout could be more cache friendly? > Rusty. I thought that used ring has 8 bytes per entry, and that struct vring_used is aligned at page boundary, this would mean that ring element is at offset 4 bytes from page boundary. Thus with cacheline size 128 bytes, each 4th element crosses a cacheline boundary. If we had a 4 byte padding after idx, each used element would always be completely within a single cacheline. What am I missing?
On Sun, 9 May 2010 06:27:33 pm Michael S. Tsirkin wrote: > On Fri, May 07, 2010 at 12:35:39PM +0930, Rusty Russell wrote: > > Then there's padding to page boundary. That puts us on a cacheline again > > for the used ring; also 2 bytes per entry. > > > > Hmm, is used ring really 2 bytes per entry? Err, no, I am an idiot. > /* u32 is used here for ids for padding reasons. */ > struct vring_used_elem { > /* Index of start of used descriptor chain. */ > __u32 id; > /* Total length of the descriptor chain which was used (written to) */ > __u32 len; > }; > > struct vring_used { > __u16 flags; > __u16 idx; > struct vring_used_elem ring[]; > }; OK, now I get it. Sorry, I was focussed on the avail ring. > I thought that used ring has 8 bytes per entry, and that struct > vring_used is aligned at page boundary, this > would mean that ring element is at offset 4 bytes from page boundary. > Thus with cacheline size 128 bytes, each 4th element crosses > a cacheline boundary. If we had a 4 byte padding after idx, each > used element would always be completely within a single cacheline. I think the numbers are: every 16th entry hits two cachelines. So currently the first 15 entries are "free" (assuming we hit the idx cacheline anyway), then 1 in 16 cost 2 cachelines. That makes the aligned version win when N > 240. But, we access the array linearly. So the extra cacheline cost is in fact amortized. I doubt it could be measured, but maybe vring_get_buf() should prefetch? While you're there, we could use an & rather than a mod on the calculation, which may actually be measurable :) Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -71,9 +71,6 @@ struct vring_virtqueue /* Number we've added since last sync. */ unsigned int num_added; - /* Last used index we've seen. */ - u16 last_used_idx; - /* How to notify other side. FIXME: commonalize hcalls! */ void (*notify)(struct virtqueue *vq); @@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt static inline bool more_used(const struct vring_virtqueue *vq) { - return vq->last_used_idx != vq->vring.used->idx; + return vring_last_used(&vq->vring) != vq->vring.used->idx; } static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len) { struct vring_virtqueue *vq = to_vvq(_vq); + struct vring_used_elem *u; void *ret; unsigned int i; @@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu return NULL; } - i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id; - *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len; + u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num]; + i = u->id; + *len = u->len; + /* Make sure we don't reload i after doing checks. */ + rmb(); if (unlikely(i >= vq->vring.num)) { BAD_RING(vq, "id %u out of range\n", i); @@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu /* detach_buf clears data, so grab it now. */ ret = vq->data[i]; detach_buf(vq, i); - vq->last_used_idx++; + vring_last_used(&vq->vring)++; + END_USE(vq); return ret; } @@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un vq->vq.name = name; vq->notify = notify; vq->broken = false; - vq->last_used_idx = 0; vq->num_added = 0; list_add_tail(&vq->vq.list, &vdev->vqs); #ifdef DEBUG @@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC); + /* We publish indices whether they offer it or not: if not, it's junk + * space anyway. But calling this acknowledges the feature. */ + virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES); + /* No callback? Tell other side not to bother us. */ if (!callback) vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT; @@ -443,6 +448,8 @@ void vring_transport_features(struct vir switch (i) { case VIRTIO_RING_F_INDIRECT_DESC: break; + case VIRTIO_RING_F_PUBLISH_INDICES: + break; default: /* We don't understand this bit. */ clear_bit(i, vdev->features); diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h --- a/include/linux/virtio_ring.h +++ b/include/linux/virtio_ring.h @@ -29,6 +29,9 @@ /* We support indirect buffer descriptors */ #define VIRTIO_RING_F_INDIRECT_DESC 28 +/* We publish our last-seen used index at the end of the avail ring. */ +#define VIRTIO_RING_F_PUBLISH_INDICES 29 + /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */ struct vring_desc { @@ -87,6 +90,7 @@ struct vring { * __u16 avail_flags; * __u16 avail_idx; * __u16 available[num]; + * __u16 last_used_idx; * * // Padding to the next align boundary. * char pad[]; @@ -95,6 +99,7 @@ struct vring { * __u16 used_flags; * __u16 used_idx; * struct vring_used_elem used[num]; + * __u16 last_avail_idx; * }; */ static inline void vring_init(struct vring *vr, unsigned int num, void *p, @@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign { return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num) + align - 1) & ~(align - 1)) - + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num; + + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2; } +/* We publish the last-seen used index at the end of the available ring, and + * vice-versa. These are at the end for backwards compatibility. */ +#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num]) +#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num]) + #ifdef __KERNEL__ #include <linux/irqreturn.h> struct virtio_device;