Hi Ohad, On Mon, Apr 22, 2013 at 3:08 PM, Ohad Ben-Cohen <ohad@xxxxxxxxxx> wrote: > Hi Sjur and Rusty, > > On Wed, Apr 10, 2013 at 12:39 AM, Sjur Brændeland > <sjur.brandeland@xxxxxxxxxxxxxx> wrote: >> Implement the vringh callback functions in order >> to manage host virito rings and handle kicks. >> This allows virtio device to request host-virtio-rings. >> >> Signed-off-by: Sjur Brændeland <sjur.brandeland@xxxxxxxxxxxxxx> > > Thanks for pushing this. > > I have a few observations which I'd like to discuss with Rusty and you. > > Rusty - will you be willing to consider a few patches to virtio which > will considerably simplify kernel users of vringh (such as this one) ? > > The main motivation is to reduce code duplication and complexity. > Right now the code handling vringh is awfully similar to the one > handling regular vrings, with a few asymmetries: > - We need to call and maintain the vrh_callback_t pointer directly, > instead of relying on vring_interrupt() > - We have some vringh creation boilerplate code of our own very > similar to what vring_new_virtqueue is doing for regular vrings > - It seems that a driver which has both regular rings and host rings > (as caif does) will have to "find_vqs" them twice - once for the > regular rings and one for the host rings - where each of those > invocations will use its own set of indices. To simplify drivers it > might be nice if we had a single find_rings function which would > enumerate both regular and host rings using a single indices axis. It > may presumably take two nvqs params, one for the regular rings and the > other for the host rings, and we can arbitrarily decide it always > creates the regular rings first. > > Stuff which will be nice to change along these lines: > > - maintain the vrh_callback_t pointer in struct vringh, similarly to > what struct virtqueue does today for callbacks of regular rings > - when kicked, just call vring_interrupt, and let it demultiplex the > event to the appropriate ring (whether it is regular or host ring) > - try to push code from rproc_create_new_vringh into virtio, following > the lines of vring_new_virtqueue and regular rings > - try to merge the regular and host rings versions of the > find_vqs/del_vqs boilerplate code to avoid duplication > > I guess this all depends on how such patches will look like and > whether we can reach an elegant implementation. I'm also aware that > host rings are being used by user space too, and we must not break > anything. Quite frankly I would *really* like to see vringh support in remoteproc included in 3.10, and I am afraid your suggestion will cause us to miss the merge window once again. How about including my simple, but verbose patch in virtio-next for 3.10. Then you could post patches that refactors remoteproc and vringh making the code more elegant and less verbose on top of my patch. Doing this iteratively would also make it easy to verify your changes, and show how your patches reduces overall code size and provides a more elegant solution. Thanks, Sjur _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
