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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?

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