On Fri, Dec 07, 2012 at 12:05:11PM +0100, Sjur BRENDELAND wrote: > 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 GCC for the in-kernel version and kernel changes for the userspace version. -- MST _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
