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

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

[Stefano Stabellini]

This patch introduces macros, structs and functions to handle rings in
the format described by docs/misc/pvcalls.markdown and
docs/misc/9pfs.markdown. The index page (struct __name##_data_intf)
contains the indexes and the grant refs to setup two rings.

               Indexes page
               +----------------------+
               |@0 $NAME_data_intf:   |
               |@76: ring_order = 1   |
               |@80: ref[0]+          |
               |@84: ref[1]+          |
               |           |          |
               |           |          |
               +----------------------+
                           |
                           v (data ring)
                   +-------+-----------+
                   |  @0->4098: in     |
                   |  ref[0]           |
                   |-------------------|
                   |  @4099->8196: out |
                   |  ref[1]           |
                   +-------------------+

$NAME_read_packet and $NAME_write_packet are provided to read or write
any data struct from/to the ring. In pvcalls, they are unused. In xen
9pfs, they are used to read or write the 9pfs header. In other protocols
they could be used to read/write the whole request structure. See
docs/misc/9pfs.markdown:Ring Usage to learn how to check how much data
is on the ring, and how to handle notifications.

There is a ring_size parameter to most functions so that protocols using
these macros don't have to have a statically defined ring order at build
time. In pvcalls for example, each new ring could have a different
order.

These macros don't help you share the indexes page or the event channels
needed for notifications. You can do that with other out of band
mechanisms, such as xenstore or another ring.

It is not possible to use a macro to define another macro with a
variable name. For this reason, this patch introduces static inline
functions instead, that are not C89 compliant. Additionally, the macro
defines a struct with a variable sized array, which is also not C89
compliant.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: konrad.wilk@oracle.com
CC: andr2000@gmail.com
CC: oleksandr_andrushchenko@epam.com
CC: andrii.anisov@gmail.com
CC: vlad.babchuk@gmail.com
CC: al1img@gmail.com
CC: joculator@gmail.com

---
Changes in v3:
- mention C89 compliance breakages
- constify parameters
- use unsigned chars for buffers
- add two macros, one doesn't define the struct

Changes in v2:
- fix typo
- remove leading underscores from names
- use UL
- do not parenthesize parameters
- code readability improvements

Give a look at the following branch to see how they are used with
pvcalls and xen-9pfs (the drivers are still work in progress):

git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git 9pfs-async-v6
---
---
 xen/include/public/io/ring.h | 120 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 801c0da..4d36f06 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -313,6 +313,126 @@ typedef struct __name##_back_ring __name##_back_ring_t
     (_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r);                  \
 } while (0)
 
+
+/*
+ * DEFINE_XEN_FLEX_RING_AND_INTF defines two monodirectional rings and
+ * functions to check if there is data on the ring, and to read and
+ * write to them.
+ *
+ * DEFINE_XEN_FLEX_RING is similar to DEFINE_XEN_FLEX_RING_AND_INTF, but
+ * does not define the indexes page. As different protocols can have
+ * extensions to the basic format, this macro allow them to define their
+ * own struct.
+ *
+ * XEN_FLEX_RING_SIZE
+ *   Convenience macro to calculate the size of one of the two rings
+ *   from the overall order.
+ *
+ * $NAME_mask
+ *   Function to apply the size mask to an index, to reduce the index
+ *   within the range [0-size].
+ *
+ * $NAME_read_packet
+ *   Function to read a defined amount of data from the ring. The amount
+ *   of data read is sizeof(__packet_t).
+ *
+ * $NAME_write_packet
+ *   Function to write a defined amount of data to the ring. The amount
+ *   of data to write is sizeof(__packet_t).
+ *
+ * $NAME_data_intf
+ *   Indexes page, shared between frontend and backend. It also
+ *   contains the array of grant refs.
+ *
+ * $NAME_queued
+ *   Function to calculate how many bytes are currently on the ring,
+ *   ready to be read. It can also be used to calculate how much free
+ *   space is currently on the ring (ring_size - $NAME_queued()).
+ */
+#define XEN_FLEX_RING_SIZE(order)                                             \
+    (1UL << (order + PAGE_SHIFT - 1))
+
+#define DEFINE_XEN_FLEX_RING_AND_INTF(name, packet_t)                         \
+struct name##_data_intf {                                                     \
+    RING_IDX in_cons, in_prod;                                                \
+                                                                              \
+    uint8_t pad1[56];                                                         \
+                                                                              \
+    RING_IDX out_cons, out_prod;                                              \
+                                                                              \
+    uint8_t pad2[56];                                                         \
+                                                                              \
+    RING_IDX ring_order;                                                      \
+    grant_ref_t ref[];                                                        \
+};                                                                            \
+DEFINE_XEN_FLEX_RING(name, packet_t);
+
+#define DEFINE_XEN_FLEX_RING(name, packet_t)                                  \
+static inline RING_IDX name##_mask(RING_IDX idx, RING_IDX ring_size)          \
+{                                                                             \
+    return (idx & (ring_size - 1));                                           \
+}                                                                             \
+                                                                              \
+static inline RING_IDX name##_mask_order(RING_IDX idx, RING_IDX ring_order)   \
+{                                                                             \
+    return (idx & (XEN_FLEX_RING_SIZE(ring_order) - 1));                      \
+}                                                                             \
+                                                                              \
+static inline void name##_read_packet(const unsigned char *buf,               \
+        RING_IDX masked_prod, RING_IDX *masked_cons,                          \
+        RING_IDX ring_size, packet_t *h) {                                    \
+    if (*masked_cons < masked_prod ||                                         \
+            sizeof(*h) <= ring_size - *masked_cons) {                         \
+        memcpy(h, buf + *masked_cons, sizeof(*h));                            \
+    } else {                                                                  \
+        memcpy(h, buf + *masked_cons, ring_size - *masked_cons);              \
+        memcpy((unsigned char *)h + ring_size - *masked_cons, buf,            \
+                sizeof(*h) - (ring_size - *masked_cons));                     \
+    }                                                                         \
+    *masked_cons = name##_mask(*masked_cons + sizeof(*h), ring_size);         \
+}                                                                             \
+                                                                              \
+static inline void name##_write_packet(unsigned char *buf,                    \
+        RING_IDX *masked_prod, RING_IDX masked_cons,                          \
+        RING_IDX ring_size, const packet_t *h) {                              \
+    if (*masked_prod < masked_cons ||                                         \
+        sizeof(*h) <= ring_size - *masked_prod) {                             \
+        memcpy(buf + *masked_prod, h, sizeof(*h));                            \
+    } else {                                                                  \
+        memcpy(buf + *masked_prod, h, ring_size - *masked_prod);              \
+        memcpy(buf, (unsigned char *)h + (ring_size - *masked_prod),          \
+                sizeof(*h) - (ring_size - *masked_prod));                     \
+    }                                                                         \
+    *masked_prod = name##_mask(*masked_prod + sizeof(*h), ring_size);         \
+}                                                                             \
+                                                                              \
+struct name##_data {                                                          \
+    unsigned char *in; /* half of the allocation */                           \
+    unsigned char *out; /* half of the allocation */                          \
+};                                                                            \
+                                                                              \
+                                                                              \
+static inline RING_IDX name##_queued(RING_IDX prod,                           \
+        RING_IDX cons, RING_IDX ring_size)                                    \
+{                                                                             \
+    RING_IDX size;                                                            \
+                                                                              \
+    if (prod == cons)                                                         \
+        return 0;                                                             \
+                                                                              \
+    prod = name##_mask(prod, ring_size);                                      \
+    cons = name##_mask(cons, ring_size);                                      \
+                                                                              \
+    if (prod == cons)                                                         \
+        return ring_size;                                                     \
+                                                                              \
+    if (prod > cons)                                                          \
+        size = prod - cons;                                                   \
+    else                                                                      \
+        size = ring_size - (cons - prod);                                     \
+    return size;                                                              \
+};
+
 #endif /* __XEN_PUBLIC_IO_RING_H__ */
 
 /*
-- 
1.9.1


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

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

[Oleksandr Andrushchenko]

Hi, Stefano, Jan!

1. Stefano, are you still NOT considering adding

functionality to avoid memory copying? We discussed

this a little bit here [1].

2. Will you also provide macros/inlines for fixed sized packets?

So, others do not reinvent the wheel again on top of your code.

3. C89 - Jan, we discussed that a bit for zero-sized arrays [2]

and empty structures [3]. So, then we came to a conclusion that

breakage is not acceptable, but now I see that you have changed

your mind? (Please correct me if I got it wrong). The reason I am

bringing this back to life is that sndif and displif protocols

use grefs[1] construct, while originally I was about to avoid that "1".

So, as now Stefano introduces *grant_ref_t ref[];* I would also

like to change sndif/displif to use the same. Do you think it can be

done this time?

Thank you,

Oleksandr

[1] https://marc.info/?l=xen-devel&m=148356651811001&w=2
[2] https://marc.info/?l=xen-devel&m=148006553214656&w=2
[3] https://marc.info/?l=xen-devel&m=148000448701946&w=2

On 02/22/2017 01:25 AM, Stefano Stabellini wrote:
> This patch introduces macros, structs and functions to handle rings in
> the format described by docs/misc/pvcalls.markdown and
> docs/misc/9pfs.markdown. The index page (struct __name##_data_intf)
> contains the indexes and the grant refs to setup two rings.
>
>                 Indexes page
>                 +----------------------+
>                 |@0 $NAME_data_intf:   |
>                 |@76: ring_order = 1   |
>                 |@80: ref[0]+          |
>                 |@84: ref[1]+          |
>                 |           |          |
>                 |           |          |
>                 +----------------------+
>                             |
>                             v (data ring)
>                     +-------+-----------+
>                     |  @0->4098: in     |
>                     |  ref[0]           |
>                     |-------------------|
>                     |  @4099->8196: out |
>                     |  ref[1]           |
>                     +-------------------+
>
> $NAME_read_packet and $NAME_write_packet are provided to read or write
> any data struct from/to the ring. In pvcalls, they are unused. In xen
> 9pfs, they are used to read or write the 9pfs header. In other protocols
> they could be used to read/write the whole request structure. See
> docs/misc/9pfs.markdown:Ring Usage to learn how to check how much data
> is on the ring, and how to handle notifications.
>
> There is a ring_size parameter to most functions so that protocols using
> these macros don't have to have a statically defined ring order at build
> time. In pvcalls for example, each new ring could have a different
> order.
>
> These macros don't help you share the indexes page or the event channels
> needed for notifications. You can do that with other out of band
> mechanisms, such as xenstore or another ring.
>
> It is not possible to use a macro to define another macro with a
> variable name. For this reason, this patch introduces static inline
> functions instead, that are not C89 compliant. Additionally, the macro
> defines a struct with a variable sized array, which is also not C89
> compliant.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: konrad.wilk@oracle.com
> CC: andr2000@gmail.com
> CC: oleksandr_andrushchenko@epam.com
> CC: andrii.anisov@gmail.com
> CC: vlad.babchuk@gmail.com
> CC: al1img@gmail.com
> CC: joculator@gmail.com
>
> ---
> Changes in v3:
> - mention C89 compliance breakages
> - constify parameters
> - use unsigned chars for buffers
> - add two macros, one doesn't define the struct
>
> Changes in v2:
> - fix typo
> - remove leading underscores from names
> - use UL
> - do not parenthesize parameters
> - code readability improvements
>
> Give a look at the following branch to see how they are used with
> pvcalls and xen-9pfs (the drivers are still work in progress):
>
> git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git 9pfs-async-v6
> ---
> ---
>   xen/include/public/io/ring.h | 120 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 120 insertions(+)
>
> diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> index 801c0da..4d36f06 100644
> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -313,6 +313,126 @@ typedef struct __name##_back_ring __name##_back_ring_t
>       (_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r);                  \
>   } while (0)
>   
> +
> +/*
> + * DEFINE_XEN_FLEX_RING_AND_INTF defines two monodirectional rings and
> + * functions to check if there is data on the ring, and to read and
> + * write to them.
> + *
> + * DEFINE_XEN_FLEX_RING is similar to DEFINE_XEN_FLEX_RING_AND_INTF, but
> + * does not define the indexes page. As different protocols can have
> + * extensions to the basic format, this macro allow them to define their
> + * own struct.
> + *
> + * XEN_FLEX_RING_SIZE
> + *   Convenience macro to calculate the size of one of the two rings
> + *   from the overall order.
> + *
> + * $NAME_mask
> + *   Function to apply the size mask to an index, to reduce the index
> + *   within the range [0-size].
> + *
> + * $NAME_read_packet
> + *   Function to read a defined amount of data from the ring. The amount
> + *   of data read is sizeof(__packet_t).
> + *
> + * $NAME_write_packet
> + *   Function to write a defined amount of data to the ring. The amount
> + *   of data to write is sizeof(__packet_t).
> + *
> + * $NAME_data_intf
> + *   Indexes page, shared between frontend and backend. It also
> + *   contains the array of grant refs.
> + *
> + * $NAME_queued
> + *   Function to calculate how many bytes are currently on the ring,
> + *   ready to be read. It can also be used to calculate how much free
> + *   space is currently on the ring (ring_size - $NAME_queued()).
> + */
> +#define XEN_FLEX_RING_SIZE(order)                                             \
> +    (1UL << (order + PAGE_SHIFT - 1))
> +
> +#define DEFINE_XEN_FLEX_RING_AND_INTF(name, packet_t)                         \
> +struct name##_data_intf {                                                     \
> +    RING_IDX in_cons, in_prod;                                                \
> +                                                                              \
> +    uint8_t pad1[56];                                                         \
> +                                                                              \
> +    RING_IDX out_cons, out_prod;                                              \
> +                                                                              \
> +    uint8_t pad2[56];                                                         \
> +                                                                              \
> +    RING_IDX ring_order;                                                      \
> +    grant_ref_t ref[];                                                        \
> +};                                                                            \
> +DEFINE_XEN_FLEX_RING(name, packet_t);
> +
> +#define DEFINE_XEN_FLEX_RING(name, packet_t)                                  \
> +static inline RING_IDX name##_mask(RING_IDX idx, RING_IDX ring_size)          \
> +{                                                                             \
> +    return (idx & (ring_size - 1));                                           \
> +}                                                                             \
> +                                                                              \
> +static inline RING_IDX name##_mask_order(RING_IDX idx, RING_IDX ring_order)   \
> +{                                                                             \
> +    return (idx & (XEN_FLEX_RING_SIZE(ring_order) - 1));                      \
> +}                                                                             \
> +                                                                              \
> +static inline void name##_read_packet(const unsigned char *buf,               \
> +        RING_IDX masked_prod, RING_IDX *masked_cons,                          \
> +        RING_IDX ring_size, packet_t *h) {                                    \
> +    if (*masked_cons < masked_prod ||                                         \
> +            sizeof(*h) <= ring_size - *masked_cons) {                         \
> +        memcpy(h, buf + *masked_cons, sizeof(*h));                            \
> +    } else {                                                                  \
> +        memcpy(h, buf + *masked_cons, ring_size - *masked_cons);              \
> +        memcpy((unsigned char *)h + ring_size - *masked_cons, buf,            \
> +                sizeof(*h) - (ring_size - *masked_cons));                     \
> +    }                                                                         \
> +    *masked_cons = name##_mask(*masked_cons + sizeof(*h), ring_size);         \
> +}                                                                             \
> +                                                                              \
> +static inline void name##_write_packet(unsigned char *buf,                    \
> +        RING_IDX *masked_prod, RING_IDX masked_cons,                          \
> +        RING_IDX ring_size, const packet_t *h) {                              \
> +    if (*masked_prod < masked_cons ||                                         \
> +        sizeof(*h) <= ring_size - *masked_prod) {                             \
> +        memcpy(buf + *masked_prod, h, sizeof(*h));                            \
> +    } else {                                                                  \
> +        memcpy(buf + *masked_prod, h, ring_size - *masked_prod);              \
> +        memcpy(buf, (unsigned char *)h + (ring_size - *masked_prod),          \
> +                sizeof(*h) - (ring_size - *masked_prod));                     \
> +    }                                                                         \
> +    *masked_prod = name##_mask(*masked_prod + sizeof(*h), ring_size);         \
> +}                                                                             \
> +                                                                              \
> +struct name##_data {                                                          \
> +    unsigned char *in; /* half of the allocation */                           \
> +    unsigned char *out; /* half of the allocation */                          \
> +};                                                                            \
> +                                                                              \
> +                                                                              \
> +static inline RING_IDX name##_queued(RING_IDX prod,                           \
> +        RING_IDX cons, RING_IDX ring_size)                                    \
> +{                                                                             \
> +    RING_IDX size;                                                            \
> +                                                                              \
> +    if (prod == cons)                                                         \
> +        return 0;                                                             \
> +                                                                              \
> +    prod = name##_mask(prod, ring_size);                                      \
> +    cons = name##_mask(cons, ring_size);                                      \
> +                                                                              \
> +    if (prod == cons)                                                         \
> +        return ring_size;                                                     \
> +                                                                              \
> +    if (prod > cons)                                                          \
> +        size = prod - cons;                                                   \
> +    else                                                                      \
> +        size = ring_size - (cons - prod);                                     \
> +    return size;                                                              \
> +};
> +
>   #endif /* __XEN_PUBLIC_IO_RING_H__ */
>   
>   /*


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

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

[Jan Beulich]

>>> On 22.02.17 at 08:16, <andr2000@gmail.com> wrote:
> 3. C89 - Jan, we discussed that a bit for zero-sized arrays [2]
> and empty structures [3]. So, then we came to a conclusion that
> breakage is not acceptable, but now I see that you have changed
> your mind? (Please correct me if I got it wrong). The reason I am
> bringing this back to life is that sndif and displif protocols
> use grefs[1] construct, while originally I was about to avoid that "1".
> So, as now Stefano introduces *grant_ref_t ref[];* I would also
> like to change sndif/displif to use the same. Do you think it can be
> done this time?

I don't think so, no. Please pay close attention to the reason why I
think an exception is fine in the case here, but not there: Someone
including ring.h without actively using the new macro will not see a
compile failure. Someone including sndif.h, otoh, would afaict.

Jan


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

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

[Oleksandr Andrushchenko]

On 02/22/2017 09:40 AM, Jan Beulich wrote:
>>>> On 22.02.17 at 08:16, <andr2000@gmail.com> wrote:
>> 3. C89 - Jan, we discussed that a bit for zero-sized arrays [2]
>> and empty structures [3]. So, then we came to a conclusion that
>> breakage is not acceptable, but now I see that you have changed
>> your mind? (Please correct me if I got it wrong). The reason I am
>> bringing this back to life is that sndif and displif protocols
>> use grefs[1] construct, while originally I was about to avoid that "1".
>> So, as now Stefano introduces *grant_ref_t ref[];* I would also
>> like to change sndif/displif to use the same. Do you think it can be
>> done this time?
> I don't think so, no. Please pay close attention to the reason why I
> think an exception is fine in the case here, but not there: Someone
> including ring.h without actively using the new macro will not see a
> compile failure. Someone including sndif.h, otoh, would afaict.
fair enough, but it means that those who want to use
PV calls/9pfs will not be able to do so with C89, right?
> Jan
>


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

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

[Jan Beulich]

>>> On 22.02.17 at 08:47, <andr2000@gmail.com> wrote:
> On 02/22/2017 09:40 AM, Jan Beulich wrote:
>>>>> On 22.02.17 at 08:16, <andr2000@gmail.com> wrote:
>>> 3. C89 - Jan, we discussed that a bit for zero-sized arrays [2]
>>> and empty structures [3]. So, then we came to a conclusion that
>>> breakage is not acceptable, but now I see that you have changed
>>> your mind? (Please correct me if I got it wrong). The reason I am
>>> bringing this back to life is that sndif and displif protocols
>>> use grefs[1] construct, while originally I was about to avoid that "1".
>>> So, as now Stefano introduces *grant_ref_t ref[];* I would also
>>> like to change sndif/displif to use the same. Do you think it can be
>>> done this time?
>> I don't think so, no. Please pay close attention to the reason why I
>> think an exception is fine in the case here, but not there: Someone
>> including ring.h without actively using the new macro will not see a
>> compile failure. Someone including sndif.h, otoh, would afaict.
> fair enough, but it means that those who want to use
> PV calls/9pfs will not be able to do so with C89, right?

They would need to customize the header to do so, or introduce a
variant of the macro Stefano proposes elsewhere.

Jan


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

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

[Stefano Stabellini]

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. I cannot introduce memory sharing
interfaces as part of this patch, because they wouldn't comply to
pvcalls or xen-9pfs 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.


> 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.

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

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

[Oleksandr Andrushchenko]

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 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.
So, my point is that we should give a possibility to directly
access ring's contents, which will not be used in your case.
>   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)

Thank you,
Oleksandr

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

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

[Stefano Stabellini]

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Oleksandr Andrushchenko]

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Stefano Stabellini]

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?

Cheers,

Stefano

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

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

[Oleksandr Andrushchenko]

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Stefano Stabellini]

On Tue, 28 Feb 2017, Oleksandr Andrushchenko wrote:
> 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

Thank you :-)


> 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"

Fair enough. It is possible to avoid the memcpy by pointing directly to
the memory on the ring:

    struct xen_example_request *h;
    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);

        h = in + masked_cons;

        do_something_with_packet(h, masked_prod, masked_cons, XEN_EXAMPLE_RING_SIZE);

        ring->intf->in_cons = cons + sizeof(*h);
        wmb();
    }

However you would have to be careful reading the data because it could
wrap around the ring. The read_packet and write_packet macros do the
calculations for you, wrapping around when necessary, but they require a
memcpy. In this example, do_something_with_packet cannot read a
sizeof(*h) amount of bytes without checking if it causes masked_cons to
go over XEN_EXAMPLE_RING_SIZE. Alternatively, you could write an sg_list
to handle the read or the write:

    struct iovec out_sg[2]; /* sg list to fill up */
    int *num;               /* size of the sg list */
    int out_size;           /* data to write */

    RING_IDX cons, prod, masked_prod, masked_cons;

    cons = intf->out_cons;
    prod = intf->out_prod;
    rmb();
    masked_prod = xen_example_mask(prod, XEN_EXAMPLE_RING_SIZE);
    masked_cons = xen_example_mask(cons, XEN_EXAMPLE_RING_SIZE);

    if (masked_cons < masked_prod) {
        out_sg[0].iov_base = out + masked_cons;
        out_sg[0].iov_len = out_size;
        *num = 1;
    } else {
        if (out_size > (XEN_EXAMPLE_RING_SIZE - masked_cons)) {
            out_sg[0].iov_base = out + masked_cons;
            out_sg[0].iov_len = XEN_EXAMPLE_RING_SIZE - masked_cons;
            out_sg[1].iov_base = out;
            out_sg[1].iov_len = out_size - (XEN_EXAMPLE_RING_SIZE - masked_cons);
            *num = 2;
        } else {
            out_sg[0].iov_base = out + masked_cons;
            out_sg[0].iov_len = out_size;
            *num = 1;
        }
    }

I hope this helps. Honestly, it looks like you wouldn't gain much by
using this approach over the traditional macros in your use cases.

Konrad, what do you think?

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