On Thu, Nov 12, 2015 at 04:38:51PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2015 at 02:34:56PM +0200, Victor Kaplansky wrote:
> > /* Based on qemu/hw/virtio/vhost-user.c */
> > @@ -173,6 +180,9 @@ typedef struct VubrVirtq {
> > #define VHOST_MEMORY_MAX_NREGIONS 8
> > #define VHOST_USER_F_PROTOCOL_FEATURES 30
> >
> > +typedef uint8_t vhost_log_chunk_t;
>
> Most code just uses uint8_t directly - I think you should
> just drop this typedef.
Oh, right. I'll clean this.
> > @@ -234,6 +249,7 @@ typedef struct VhostUserMsg {
> > struct vhost_vring_state state;
> > struct vhost_vring_addr addr;
> > VhostUserMemory memory;
> > + VhostUserLog log;
> > } payload;
> > int fds[VHOST_MEMORY_MAX_NREGIONS];
> > int fd_num;
> > @@ -265,8 +281,13 @@ typedef struct VubrDev {
> > uint32_t nregions;
> > VubrDevRegion regions[VHOST_MEMORY_MAX_NREGIONS];
> > VubrVirtq vq[MAX_NR_VIRTQUEUE];
> > + int log_call_fd;
>
> This doesn't seem to be used. Pls add code to signal this
> after logging.
>
Right, implementation of SET_LOG_FD request handler was on my TODO
list. I'll include it in the next version of the patch (as well
as using it for the signaling).
> > @@ -465,12 +496,39 @@ vubr_virtqueue_kick(VubrVirtq *vq)
> > }
> > }
> >
> > +
> > +static void
> > +vubr_log_page(uint8_t *log_table, uint64_t page)
> > +{
> > + DPRINT("Logged dirty guest page: %"PRId64"\n", page);
> > + log_table[page / 8] |= 1 << (page % 8);
> > +}
> > +
>
> This is only safe if there's a single writer.
> Please add a comment that says as much.
> Or set this atomically, that's also not hard to do.
>
I'll set it atomically.
> > +static void
> > +vubr_log_write(VubrDev *dev, uint64_t address, uint64_t length)
> > +{
> > + uint64_t page;
> > +
> > + if (!(dev->features & VHOST_F_LOG_ALL) || !dev->log_table || !length) {
> > + return;
> > + }
> > +
> > + assert(dev->log_size >= ((address + length) / VHOST_LOG_PAGE / 8));
>
> I think there's an off by 1 here.
> Imagine size == 0, test should always fail.
> But imagine address == 0 and length == 1.
> You get >= and test passes, seems wrong.
>
Oh, good catch. Will fix it. Hopefully, right way this time. ;-)
> > + rc = mmap(0, log_mmap_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
> > + log_mmap_offset);
> > + if (rc == MAP_FAILED) {
> > + vubr_die("mmap");
> > + }
> > + dev->log_table = (vhost_log_chunk_t *) rc;
>
> Cast is not needed here.
>
Indeed.
> > @@ -829,6 +956,10 @@ vubr_set_vring_kick_exec(VubrDev *dev, VhostUserMsg
> > *vmsg)
> > DPRINT("Waiting for kicks on fd: %d for vq: %d\n",
> > dev->vq[index].kick_fd, index);
> > }
> > + if (dev->vq[0].kick_fd != -1 &&
> > + dev->vq[1].kick_fd != -1) {
> > + dev->ready = 1;
>
> I'm not sure what this does. Related to logging somehow?
> Anyway, processing a VQ should happen after either
> - you received a kick
> or
> - you received VRING_ENABLE
It is a temporarily hack to determine if vrings are ready for
processing. AFAIR, DPDK code uses the same heuristics. I'll add an
explanation in the comments.
--Victor
