Re: [Xen-devel] Re: [Qemu-devel] [PATCH 0/7] merge some xen bits into qemu

[Samuel Thibault <samuel.thibault@eu.citrix.com>]

Gerd Hoffmann, le Thu 07 Aug 2008 14:13:22 +0200, a écrit :
> > Any reason for the renames, though? (they tend to bother developpers who
> > have to change their habits, so we can not do that without a reason)
> 
> Get consistent naming (all xen stuff in hw/ is prefixed with xen-).

Err, no, in xen they are all prefixed with xen_ (except xenfb).

> > - Why dashes instead of underscores?
> 
> (1) It looks more pleasent to my eyes.
> (2) It is easier to type (no shift needed) on both us and german
>     keyboards.

Not on fr keyboard :p

> (3) The files in the qemu source tree don't have a consistent style
>     in respect to '-' vs. '_',

There are far more _ than - in qemu. - seems to be only used for
things that just share a very generic idea (i.e. usb- and scsi-), while
_ seems to be used for things that are more closely related, like arm_*,
mips_*, ppc_*, ... xen_* would make sense to my mind.

> so I had no reason to not use my personal preference ;)

Yes, there is a reason: as I said, that puts a little burden on
developpers that have already been working on it in Xen for some time.
That also asks Ian to do the move, that makes history digging more
tricky, etc.

> I'd prefer to stay with the xen-machine.c though.

Ok for your taste, but as I said the burden involved by renamings looks
more important to me.

> > Misc stuff:
> > - I guess the sys-queue.h file should be merged and used in qemu first?
> 
> Rebase.  Then it will be there, and you can skip the patch ;)

Ah ok, sorry, then fine.

> > - The BUFSIZE macro in xen-backend.h is a bit unfortunate, but still
> > better than what we have currently.  Maybe it should be renamed with a
> > - XEN_ prefix to avoid any clash?
> 
> xen-backend.h is supposed to be included by xen-related code only (it
> barfs when /usr/include/xen isn't there), so it shouldn't clash in
> theory.

In theory yes, but even people working on the xen part could want to
define a very local BUFSIZE, and then they could get a clash. Of course
they can easily fix their code, but as you say

> Well, I can prefix it nevertheless, better safe than sorry.

> > - container_of would probably be useful in other parts of qemu, I guess
> > it could be merged in qemu first?
> 
> Has qemu a nice place for that kind of helper macros?

I'll leave that one for more experienced qemu people.

> > In your xen patch, since idle and GUI_REFRESH_INTERVAL are there, you
> > can just use them.  We'll push them to qemu.  We'll manage the _shared
> > stuff (and push it eventually).
> 
> I'd prefer to do it the other way around (push depending changes
> upstream, then adapt xen-framebuffer.c).  I want xen-framebuffer.c look
> the same in xen and upstream.

Sure, it's just that your code could be merged earlier with these
modifications.  But well we can handle that later it's not a problem.

> > I'll let Markus comment on the rest (again, thanks for moving the xenbus
> > stuff to the backend part).  There is one change that is not backend
> > changes or just moving code around: you are queuing rectangle updates,
> > why?  (I'm not arguing, just wondering the kind of optimization that can
> > be).
> 
> Thats actually a bugfix.  Doing screen updates outside the ->update
> callback isn't safe.

Oh.  Well, that's the kind of comments which you could have put
somewhere along your patches, for us to not frown on it while
reviewing...

For more performance, maybe it'd be better to only move the dpy_update()
part. It's better to do the xenfb_guest_copy() immediately since the
source data is probably already hot in the cache.

> >> The block backend should be a good replacement for blktap though and
> >> maybe can save you the effort of porting the blktap kernel driver to
> >> the pv_ops kernel.
> > 
> > Well, for better performance an in-kernel blktap would still be useful.
> 
> Don't confuse blkback and blktap.  I don't want to replace the in-kernel
> blkback driver.  blktap isn't a in-kernel driver though.

Well, actually _you_ confused me about this above :)

> My backend driver does pretty much the same, but uses the generic
> gntdev interface instead of blktap.  I can't see any reason why it
> shouldn't match blktap performance-wise.

That's possible indeed.

> I have no benchmarks numbers though.

That'd be good :)

Samuel