Re: [Xen-devel] [PATCH v3] ring.h: introduce macros to handle monodirectional rings with multiple req sizes

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 02/23/2017 08:45 PM, Stefano Stabellini wrote:
> On Thu, 23 Feb 2017, Oleksandr Andrushchenko wrote:
> > Hi, Stefano!
> >
> > On 02/22/2017 07:10 PM, Stefano Stabellini wrote:
> > > On Wed, 22 Feb 2017, Oleksandr Andrushchenko wrote:
> > > > Hi, Stefano, Jan!
> > > > 1. Stefano, are you still NOT considering adding
> > > > functionality to avoid memory copying? We discussed
> > > > this a little bit here [1].
> > > Hi Oleksandr,
> > >
> > > these macros are the generic versions of what pvcalls and xen-9pfs need,
> > > and these protocols use memory copies.
> > These macros will live in generic ring.h. It means that
> > they can be used not only by pvcalls/9pfs, but others are
> > allowed to use them too (kbdif/fbif/displif/??).
> That's right, but without an additional use case and test case (code I
> can test them with), it doesn't make sense for me to add more functions
> in this patch now. They would likely not be optimal. Of course they can
> be added later.
>
>
> > That being said, I am not convinced that this is a good idea
> > to introduce a memory copying while dealing with the packets
> > in an IRQ handler, for example.
> Let me premise that I have nothing against memory sharing based
> protocols, in fact I welcome them, but these macros are meant for memory
> copy based protocols. (It is a pity that the Intel and ARM architecture
> differ so much in this regard, copy being faster than mapping in most
> cases on Intel, given the high cost of tlb flushes).
>
>
> > So, my point is that we should give a possibility to directly
> > access ring's contents, which will not be used in your case.
> That I can do. I could add a simple macro to make it easy to get a
> pointer to the content for reading or writing. Today I am accessing it
> with something like:
>      
>      data->out + pvcalls_mask(intf->out_prod, array_size);
> But I could add some sugar around it.
>
>
> > >    I cannot introduce memory sharing
> > > interfaces as part of this patch, because they wouldn't comply to
> > > pvcalls or xen-9pfs
> > Again, you are thinking of pvcalls/9pfs, but none of the macros
> > say they are for these use-cases: anyone can use them
> > >    and I don't have another test case for them.
> > > But if you had a patch to introduce them in ring.h, I would be happy to
> > > help you review it.
> > No, I don't have any, sorry
> > > > 2. Will you also provide macros/inlines for fixed sized packets?
> > > > So, others do not reinvent the wheel again on top of your code.
> > > I thought I already did: you can read/write fixed sized packets using
> > > the two read/write_packet functions.
> > I was thinking of something like we have for req/resp
> >      DEFINE_RING_TYPES(fsif, struct fsif_request, struct fsif_response);
> > so you don't need to care of sizeof(req/resp/event)
> I think that would be very restrictive: with the read/write_packet
> functions you can actually read and write packets of the same type or
> different types. You could read packets of different sizes. In fact
> maybe, instead of a packet_t type parameter, we should pass an opaque
> struct pointer and a size, so that they can be used to actually read and
> write anything. That way, you get the maximum flexibility out of them.
>
> Also, if you need a traditional ring, what not use the traditional macro
> for it?
I use it for req/resp (front->back), but I also need an asynchronous event
channel (back->front)
For that reason, Konrad suggested that I can probably re-use what you
do for pvcalls/9pfs [1]. But, now I am not convinced that I should drop
what I already have in displif and what kbdif/fbif use.

Thank you,
Oleksandr

[1] https://marc.info/?l=xen-devel&m=148734926706859&w=2

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel