Re: [Qemu-devel] [PATCH v2] tests/vhost-user-bridge: implement logging of dirty pages

[Victor Kaplansky]

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