|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/3] [RESEND] ring.h: introduce macros to handle monodirectional rings with multiple req sizes
On Tue, 21 Mar 2017, Konrad Rzeszutek Wilk wrote:
> 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 ?
I don't know where 4098 and 8196 came from. Yes, they should be 4095 and
8191, I'll fix the commit message.
>
> > | 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@xxxxxxxxxxx>
> > CC: konrad.wilk@xxxxxxxxxx
> >
> > ---
> > 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?
Yes, makes sense.
> > +
> > +#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.
In inline functions we don't need to parenthesize parameter name uses.
(It was Jan to point that out actually.)
> > +}
> > \
> > +
> > \
> > +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
Sure
> > + if (*masked_cons < masked_prod ||
> > \
>
> Any particular reason you are using an pointer to masked_cons?
>
> And vice versa on the _write_packet function?
Yes: the function updates masked_cons, increasing it by the number of
bytes read. Similarly the _write_packet function increases mask_prod by
the number of bytes written.
> > + 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
Sure
> > + 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |