|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] ring.h: introduce macros to handle monodirectional rings with multiple req sizes
On Mon, 20 Feb 2017, Jan Beulich wrote:
> >>> On 17.02.17 at 23:38, <sstabellini@xxxxxxxxxx> wrote:
>
> For all of the comments below, please understand that I'm giving
> them in the understanding that pre-existing code may be similarly
> problematic; we shouldn't introduce new issues though.
I understand, thanks for the many useful comments
> > --- a/xen/include/public/io/ring.h
> > +++ b/xen/include/public/io/ring.h
> > @@ -313,6 +313,128 @@ typedef struct __name##_back_ring __name##_back_ring_t
> > (_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r); \
> > } while (0)
> >
> > +
> > +/*
> > + * DEFINE_XEN_FLEX_RING defines two monodirectional rings and functions
> > + * to check if there is data on the ring, and to read and write to them.
> > + *
> > + * 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. Different protocols can have
> > + * extensions to the basic format, in such cases please define your
> > + * own data_intf struct.
> > + *
> > + * $NAME_queued
> > + * Function to calculate how many bytes are currently on the ring,
> > + * read to be read. It can also be used to calculate how much free
>
> ready to be ...
OK
> > + * space is currently on the ring (ring_size - $NAME_queued()).
> > + */
> > +#define XEN_FLEX_RING_SIZE(__order)
> > \
> > + ((1 << ((__order) + XEN_PAGE_SHIFT)) / 2)
>
> Double-underscore prefixed names are reserved. Please avoid using
> leading underscores namely in public headers.
OK
> Also, while likely not relevant for the near future, may I suggest to
> use 1UL here, or even (size_t)1 (to also cope with P64 environments)?
> Further, with PAGE_SHIFT never zero, I think the expression would
> better be
>
> (1UL << ((__order) + XEN_PAGE_SHIFT - 1))
OK
> > +#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));
> > \
>
> In an inline function you don't need to parenthesize parameter
> name uses.
OK
> But the use of inline functions here is questionable
> in the first place - so far there are none, as they're not standard
> C89. Granted they are being used in a macro only, so won't cause
> immediate issues for code not using the macros, but anyway.
I did it because it is not possible to use a macro to define another
macro with a variable name. In other words, the following does not work:
#define DEFINE_XEN_FLEX_RING(name, packet_t) \
#define name##_mask(idx, ring_size) (idx & (ring_size - 1))
I prefer to drop C89 compliance to keep cleaner code, but if you have
better suggestions, I would be happy to hear them.
> > +static inline void __name##_read_packet(char *buf,
> > \
>
> const
We cannot use const char *buf, because we are about to pass buf to
memcpy.
> > + RING_IDX *masked_prod, RING_IDX *masked_cons,
> > \
>
> You don't update *masked_prod - why do you need a pointer here?
When using this functions in pvcalls and xen-9pfs, I found that it was
less confusing for me if the two index parameters had the same type. In
fact, I started with what you suggested, then changed it back to this.
> > + RING_IDX ring_size, __packet_t *h) {
> > \
> > + if (*masked_cons < *masked_prod) {
> > \
> > + memcpy(h, buf + *masked_cons, sizeof(*h));
> > \
> > + } else {
> > \
> > + if (sizeof(*h) > ring_size - *masked_cons) {
> > \
> > + memcpy(h, buf + *masked_cons, ring_size - *masked_cons);
> > \
> > + memcpy((char *)h + ring_size - *masked_cons, buf,
> > \
> > + sizeof(*h) - (ring_size - *masked_cons));
> > \
> > + } else {
> > \
> > + memcpy(h, buf + *masked_cons, sizeof(*h));
> > \
>
> This is the same as the first memcpy(). Care to adjust (merge) the
> conditionals so you need this only once?
Sure
> > +static inline void __name##_write_packet(char *buf,
> > \
> > + RING_IDX *masked_prod, RING_IDX *masked_cons,
> > \
> > + RING_IDX ring_size, __packet_t h) {
> > \
>
> Passing a (possibly large) structure by value? Apart from that similar
> comments as for read apply here.
It makes sense to use a pointer here, I'll do that.
> > +struct __name##_data {
> > \
> > + char *in; /* half of the allocation */
> > \
> > + char *out; /* half of the allocation */
> > \
>
> Why plain char? Void would seem the most neutral option here, but
> if arithmetic needs doing inside the header, then unsigned char may
> need to be used.
The types here should match the type of the "buf" parameter used in the
read/write_packet functions. We cannot use void as we do arithmetic. If
you think unsigned char is more appropriate, I'll also change the type
of the buf arguments elsewhere.
> > +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[];
> > \
> > +};
> > \
>
> Above you talk about the option of defining private _data_intf
> structures. How is that supposed to work when you define a
> suitably named structure already here? Other than #define-s
> one can't undo such a type definition.
Yes, but it is always possible to use a different name. For example,
this is what I did in the pvcalls header:
struct __pvcalls_data_intf {
RING_IDX in_cons, in_prod, in_error;
uint8_t pad1[52];
RING_IDX out_cons, out_prod, out_error;
uint8_t pad2[52];
RING_IDX ring_order;
grant_ref_t ref[];
};
This struct is only here as a reference, we don't have to actually
define it in ring.h. If you think it's best, I could introduce it only
as a comment.
> > +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;
> > \
> > + size += prod;
> > \
> > + }
> >
>
> To me this would read easier (due to matching up better with the
> if() path) as
>
> else
> size = ring_size - (cons - prod);
OK, I'll make the change.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |