Hi Michael, > From: Michael S. Tsirkin [mailto:mst@xxxxxxxxxx] > Sent: Thursday, December 06, 2012 12:16 PM > On Thu, Dec 06, 2012 at 12:03:43PM +0100, Sjur BRENDELAND wrote: > > Hi Michael, > > > > > > -struct vring_used_elem *vring_add_used_user(struct vring_host *vh, > > > > - unsigned int head, int len) > > > > + > > > > +static inline struct vring_used_elem *_vring_add_used(struct > vring_host > > > *vh, > > > > + u32 head, u32 len, > > > > + bool (*cpy)(void *dst, > > > > + void *src, > > > > + size_t s), > > > > + void (*wbarrier)(void)) > > > > { > > > > struct vring_used_elem *used; > > > > + u16 last_used; > > > > > > > > /* The virtqueue contains a ring of used buffers. Get a pointer to the > > > > * next entry in that used ring. */ > > > > - used = &vh->vr.used->ring[vh->last_used_idx % vh->vr.num]; > > > > - if (__put_user(head, &used->id)) { > > > > - pr_debug("Failed to write used id"); > > > > + used = &vh->vr.used->ring[vh->last_used_idx & (vh->vr.num - 1)]; > > > > + if (!cpy(&used->id, &head, sizeof(used->id)) || > > > > + !cpy(&used->len, &len, sizeof(used->len))) > > > > return NULL; > > > > - } > > > > - if (__put_user(len, &used->len)) { > > > > - pr_debug("Failed to write used len"); > > > > + wbarrier(); > > > > + last_used = vh->last_used_idx + 1; > > > > + if (!cpy(&vh->vr.used->idx, &last_used, sizeof(vh->vr.used->idx))) > > > > return NULL; > > > > > > I think this is broken: we need a 16 bit access, this is > > > doing a memcpy which is byte by byte. > > > > I have played around with gcc and -O2 option, and it seems to me > > that GCC is smart enough to optimize the use of sizeof and memcpy > > into MOV operations. But my assembly knowledge is very rusty, > > so a second opinion on this would be good. > > Yes but I don't think we should rely on this: this API is not guaranteed > to do the right thing and no one will bother telling us if it's > rewritten or something. What is your concern here Michael? Are you uncomfortable with relying on GCC optimizations of memory access and the use of inline function pointers? Or are you afraid that virtio_ring changes implementation without your knowledge, so that vhost performance will suffer? If the latter is your concern, perhaps we could implement the memory access functions in vhost.c and the shared ring functions in a header file. In this way we could still use inline function pointers as Rusty suggested, but you gain more control of the memory access from vhost. If you are concerned about GCC optimization, I can do a re-spin with your proposal using #defines... Regards, Sjur _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
