From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Stefano Stabellini <stefano@aporeto.com>, xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 1/3] [RESEND] ring.h: introduce macros to handle monodirectional rings with multiple req sizes
Date: Tue, 21 Mar 2017 15:42:42 -0400 [thread overview]
Message-ID: <20170321194242.GF23849@char.us.oracle.com> (raw)
In-Reply-To: <1490032163-21657-1-git-send-email-sstabellini@kernel.org>
On Mon, Mar 20, 2017 at 10:49:21AM -0700, 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 |
4095
> | ref[0] |
> |-------------------|
> | @4099->8196: out |
4096->8191 ?
> | 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
>
> ---
> Changes in v4:
> - remove packet_t, use void* and size instead
>
> 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-v7
> ---
> ---
> xen/include/public/io/ring.h | 131 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 131 insertions(+)
>
> diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> index 801c0da..8ac9ca3 100644
> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -313,6 +313,137 @@ 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 data from the ring. The amount of data to read is
> + * specified by the "size" argument.
> + *
> + * $NAME_write_packet
> + * Function to write data to the ring. The amount of data to write is
> + * specified by the "size" argument.
> + *
> + * $NAME_get_ring_ptr
> + * Convenience function that returns a pointer to read/write to the
> + * ring at the right location.
> + *
> + * $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) \
> +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);
Should this macro #define DEFINE_XEN_FLEX_RING_AND_INTF be below the macro
DEFINE_XEN_FLEX_RING?
As the DEFINE_XEN_FLEX_RING_AND_INTF uses the DEFINE_XEN_FLEX_RING?
> +
> +#define DEFINE_XEN_FLEX_RING(name) \
> +static inline RING_IDX name##_mask(RING_IDX idx, RING_IDX ring_size) \
> +{ \
> + return (idx & (ring_size - 1)); \
Could you put () around ring_size and idx please.
> +} \
> + \
> +static inline RING_IDX name##_mask_order(RING_IDX idx, RING_IDX ring_order) \
> +{ \
> + return (idx & (XEN_FLEX_RING_SIZE(ring_order) - 1)); \
As well for idx here?
> +} \
> + \
> +static inline unsigned char* name##_get_ring_ptr(unsigned char *buf, \
> + RING_IDX idx, \
> + RING_IDX ring_order) \
> +{ \
> + return buf + name##_mask_order(idx, ring_order); \
> +} \
> + \
> +static inline 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) { \
How about anewline here
> + if (*masked_cons < masked_prod || \
Any particular reason you are using an pointer to masked_cons?
And vice versa on the _write_packet function?
> + size <= ring_size - *masked_cons) { \
> + memcpy(opaque, buf + *masked_cons, size); \
> + } else { \
> + memcpy(opaque, buf + *masked_cons, ring_size - *masked_cons); \
> + memcpy((unsigned char *)opaque + ring_size - *masked_cons, buf, \
> + size - (ring_size - *masked_cons)); \
> + } \
> + *masked_cons = name##_mask(*masked_cons + size, ring_size); \
> +} \
> + \
> +static inline 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) { \
How about anewline here
> + if (*masked_prod < masked_cons || \
> + size <= ring_size - *masked_prod) { \
> + memcpy(buf + *masked_prod, opaque, size); \
> + } else { \
> + memcpy(buf + *masked_prod, opaque, ring_size - *masked_prod); \
> + memcpy(buf, (unsigned char *)opaque + (ring_size - *masked_prod), \
> + size - (ring_size - *masked_prod)); \
> + } \
> + *masked_prod = name##_mask(*masked_prod + size, 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
next prev parent reply other threads:[~2017-03-21 19:42 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-20 17:48 [PATCH v2 0/3] new ring macros, 9pfs and pvcalls headers Stefano Stabellini
2017-03-20 17:49 ` [PATCH v2 1/3] [RESEND] ring.h: introduce macros to handle monodirectional rings with multiple req sizes Stefano Stabellini
2017-03-20 17:49 ` [PATCH v2 2/3] Introduce the Xen 9pfs transport header Stefano Stabellini
2017-03-20 17:49 ` [PATCH v2 3/3] Introduce the pvcalls header Stefano Stabellini
2017-03-21 19:42 ` Konrad Rzeszutek Wilk [this message]
2017-03-21 20:50 ` [PATCH v2 1/3] [RESEND] ring.h: introduce macros to handle monodirectional rings with multiple req sizes Stefano Stabellini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170321194242.GF23849@char.us.oracle.com \
--to=konrad.wilk@oracle.com \
--cc=sstabellini@kernel.org \
--cc=stefano@aporeto.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.