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