>>> On 17.02.17 at 23:38, <sstabell...@kernel.org> 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.
> --- 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 ...
> + * 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.
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))
> +#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. 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.
> +static inline void __name##_read_packet(char *buf,
> \
const
> + RING_IDX *masked_prod, RING_IDX *masked_cons,
> \
You don't update *masked_prod - why do you need a pointer here?
> + 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?
> +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.
> +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.
> +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.
> +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);
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
