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

[Oleksandr Andrushchenko <andr2000@gmail.com>]

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