[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3] ring.h: introduce macros to handle monodirectional rings with multiple req sizes
On 02/28/2017 12:17 AM, Stefano Stabellini wrote:
On Mon, 27 Feb 2017, Oleksandr Andrushchenko wrote:
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.
I see. If you use these macros, you'll end up with two mono-directional
rings. One for back->front communications and the other for front->back
communications. You'll also end up with two event channels, one for each
ring. They are used to notify the other end, both producer and consumer
do that. Finally, you'll get two functions (in my latest draft, still to
be sent out):
void $NAME_read_packet(const unsigned char *buf,
RING_IDX masked_prod, RING_IDX *masked_cons,
RING_IDX ring_size, void *opaque, size_t size);
void $NAME_write_packet(unsigned char *buf,
RING_IDX *masked_prod, RING_IDX masked_cons,
RING_IDX ring_size, const void *opaque, size_t size);
You can use them to send or receive pretty much anything on the ring,
including request/response structures, but also raw data. The code that
uses them looks like this (frontend side):
struct xen_example_data_intf *intf; /* pointer to the indexes page */
unsigned char *in; /* pointer to ring buffer */
int irq; /* irq number corresponding to event
channel */
struct xen_example_request h; /* opaque struct to read, could be
your request struct */
RING_IDX cons, prod, masked_cons, masked_prod;
while (1) {
cons = intf->in_cons;
prod = intf->in_prod;
mb();
if (xen_example_queued(prod, cons, XEN_EXAMPLE_RING_SIZE) < sizeof(h)) {
notify_remote_via_irq(irq);
return;
}
masked_prod = xen_example_mask(prod, XEN_EXAMPLE_RING_SIZE);
masked_cons = xen_example_mask(cons, XEN_EXAMPLE_RING_SIZE);
xen_example_read_packet(in,
masked_prod,
&masked_cons,
XEN_EXAMPLE_RING_SIZE,
&h,
sizeof(h));
do_something_with_packet(&h);
ring->intf->in_cons = cons + sizeof(h);
wmb();
}
This is an example of the similar code dealing with variable length
request structs:
struct xen_example_data_intf *intf; /* pointer to the indexes page */
unsigned char *in; /* pointer to ring buffer */
int irq; /* irq number corresponding to event
channel */
struct xen_example_header h; /* header */
unsigned char *buf = NULL;
RING_IDX cons, prod, masked_cons, masked_prod;
while (1) {
cons = intf->in_cons;
prod = intf->in_prod;
mb();
if (xen_example_queued(prod, cons, XEN_EXAMPLE_RING_SIZE) < sizeof(h)) {
notify_remote_via_irq(irq);
return;
}
masked_prod = xen_example_mask(prod, XEN_EXAMPLE_RING_SIZE);
masked_cons = xen_example_mask(cons, XEN_EXAMPLE_RING_SIZE);
xen_example_read_packet(in,
masked_prod,
&masked_cons,
XEN_EXAMPLE_RING_SIZE,
&h,
sizeof(h));
buf = kmalloc(sizeof(h), GFP_KERNEL);
ASSERT(buf != NULL);
/* Assuming that the raw data follows the header and that h.size
* is the length of the data. */
xen_example_read_packet(in,
masked_prod,
&masked_cons,
XEN_EXAMPLE_RING_SIZE,
buf,
h.size);
do_something_with_packet(&h, buf, h.size);
ring->intf->in_cons = cons + sizeof(h) + h.size;
wmb();
kfree(buf);
}
This code is not even compile tested but I hope it helps! Would this
scheme work for you? If not, why?
At first site it will work, thank you for such a detailed explanation
But, what we already have in ring.h for req/resp case looks simpler
for my use-case: already fixed size, no memcpy
For "back->front" direction - probably I can use your approach, but
the only benefit I get is that your macros will be the part of ring.h
That being said:
1. Your approach is an excellent fit for arbitrary sized structures and
buffer size - definitely WIN
2. For my use-case I wold prefer what I already have because of
- simplicity for fixed size
- no memcpy
- possibility to put "back->front" path into the same page used for
"front->back"
Cheers,
Stefano
Thank you,
Oleksandr
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
