| Message ID | 1487371099-21988-1-git-send-email-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 17 Feb 2017, 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 | > | ref[0] | > |-------------------| > | @4099->8196: out | > | 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. > > Signed-off-by: Stefano Stabellini <stefano@aporeto.com> > CC: konrad.wilk@oracle.com > CC: andr2000@gmail.com > CC: oleksandr_andrushchenko@epam.com > CC: andrii.anisov@gmail.com > CC: vlad.babchuk@gmail.com > CC: al1img@gmail.com > CC: joculator@gmail.com > > --- > 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-v4 > --- > --- > xen/include/public/io/ring.h | 122 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 122 insertions(+) > > diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h > index 801c0da..ca9a8f0 100644 > --- 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 > + * space is currently on the ring (ring_size - $NAME_queued()). > + */ > +#define XEN_FLEX_RING_SIZE(__order) \ > + ((1 << ((__order) + XEN_PAGE_SHIFT)) / 2) This should be PAGE_SHIFT. It's XEN_PAGE_SHIFT when used as a linux header. > +#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)); \ > +} \ > + \ > +static inline RING_IDX __name##_mask_order(RING_IDX idx, RING_IDX ring_order) \ > +{ \ > + return ((idx) & (XEN_FLEX_RING_SIZE(ring_order) - 1)); \ > +} \ > + \ > +static inline void __name##_read_packet(char *buf, \ > + RING_IDX *masked_prod, RING_IDX *masked_cons, \ > + 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)); \ > + } \ > + } \ > + *masked_cons = __name##_mask(*masked_cons + sizeof(*h), ring_size); \ > +} \ > + \ > +static inline void __name##_write_packet(char *buf, \ > + RING_IDX *masked_prod, RING_IDX *masked_cons, \ > + RING_IDX ring_size, __packet_t h) { \ > + if (*masked_prod < *masked_cons) { \ > + memcpy(buf + *masked_prod, &h, sizeof(h)); \ > + } else { \ > + if (sizeof(h) > ring_size - *masked_prod) { \ > + memcpy(buf + *masked_prod, &h, ring_size - *masked_prod); \ > + memcpy(buf, (char *)(&h) + (ring_size - *masked_prod), \ > + sizeof(h) - (ring_size - *masked_prod)); \ > + } else { \ > + memcpy(buf + *masked_prod, &h, sizeof(h)); \ > + } \ > + } \ > + *masked_prod = __name##_mask(*masked_prod + sizeof(h), ring_size); \ > +} \ > + \ > +struct __name##_data { \ > + char *in; /* half of the allocation */ \ > + char *out; /* half of the allocation */ \ > +}; \ > + \ > +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[]; \ > +}; \ > + \ > +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; \ > + } \ > + return size; \ > +}; > + > #endif /* __XEN_PUBLIC_IO_RING_H__ */ > > /* > -- > 1.9.1 >
>>> On 17.02.17 at 23:38, <sstabellini@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
On Mon, 20 Feb 2017, Jan Beulich wrote: > >>> On 17.02.17 at 23:38, <sstabellini@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. 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.
>>> On 21.02.17 at 00:26, <sstabellini@kernel.org> wrote: > On Mon, 20 Feb 2017, Jan Beulich wrote: >> >>> On 17.02.17 at 23:38, <sstabellini@kernel.org> wrote: >> 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. Well, okay then, but please add a respective remark to the commit message then. >> > +static inline void __name##_read_packet(char *buf, \ >> >> const > > We cannot use const char *buf, because we are about to pass buf to > memcpy. I don't understand: memcpy()'s second parameter is const. >> > + 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. I don't find this a convincing argument. The parameters will differ in constness anyway due to the different transfer direction, and using an extra layer of indirection just for aesthetic reasons seems at least odd to me. >> > +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. Yes please - plain char is mainly meant for use with strings, not arbitrary binary data. >> > +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. How about having two variants of the macro - one (the base) version without this, and a second invoking the first and adding this? Jan
On Tue, 21 Feb 2017, Jan Beulich wrote: > >>> On 21.02.17 at 00:26, <sstabellini@kernel.org> wrote: > > On Mon, 20 Feb 2017, Jan Beulich wrote: > >> >>> On 17.02.17 at 23:38, <sstabellini@kernel.org> wrote: > >> 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. > > Well, okay then, but please add a respective remark to the commit > message then. OK > >> > +static inline void __name##_read_packet(char *buf, \ > >> > >> const > > > > We cannot use const char *buf, because we are about to pass buf to > > memcpy. > > I don't understand: memcpy()'s second parameter is const. Ah, right > >> > + 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. > > I don't find this a convincing argument. The parameters will differ > in constness anyway due to the different transfer direction, and > using an extra layer of indirection just for aesthetic reasons seems > at least odd to me. All right, I'll make the change > >> > +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. > > Yes please - plain char is mainly meant for use with strings, not > arbitrary binary data. OK > >> > +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. > > How about having two variants of the macro - one (the base) > version without this, and a second invoking the first and adding > this? Sure
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h index 801c0da..ca9a8f0 100644 --- 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 + * space is currently on the ring (ring_size - $NAME_queued()). + */ +#define XEN_FLEX_RING_SIZE(__order) \ + ((1 << ((__order) + XEN_PAGE_SHIFT)) / 2) + +#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)); \ +} \ + \ +static inline RING_IDX __name##_mask_order(RING_IDX idx, RING_IDX ring_order) \ +{ \ + return ((idx) & (XEN_FLEX_RING_SIZE(ring_order) - 1)); \ +} \ + \ +static inline void __name##_read_packet(char *buf, \ + RING_IDX *masked_prod, RING_IDX *masked_cons, \ + 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)); \ + } \ + } \ + *masked_cons = __name##_mask(*masked_cons + sizeof(*h), ring_size); \ +} \ + \ +static inline void __name##_write_packet(char *buf, \ + RING_IDX *masked_prod, RING_IDX *masked_cons, \ + RING_IDX ring_size, __packet_t h) { \ + if (*masked_prod < *masked_cons) { \ + memcpy(buf + *masked_prod, &h, sizeof(h)); \ + } else { \ + if (sizeof(h) > ring_size - *masked_prod) { \ + memcpy(buf + *masked_prod, &h, ring_size - *masked_prod); \ + memcpy(buf, (char *)(&h) + (ring_size - *masked_prod), \ + sizeof(h) - (ring_size - *masked_prod)); \ + } else { \ + memcpy(buf + *masked_prod, &h, sizeof(h)); \ + } \ + } \ + *masked_prod = __name##_mask(*masked_prod + sizeof(h), ring_size); \ +} \ + \ +struct __name##_data { \ + char *in; /* half of the allocation */ \ + char *out; /* half of the allocation */ \ +}; \ + \ +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[]; \ +}; \ + \ +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; \ + } \ + return size; \ +}; + #endif /* __XEN_PUBLIC_IO_RING_H__ */ /*
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 | | ref[0] | |-------------------| | @4099->8196: out | | 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. Signed-off-by: Stefano Stabellini <stefano@aporeto.com> CC: konrad.wilk@oracle.com CC: andr2000@gmail.com CC: oleksandr_andrushchenko@epam.com CC: andrii.anisov@gmail.com CC: vlad.babchuk@gmail.com CC: al1img@gmail.com CC: joculator@gmail.com --- 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-v4 --- --- xen/include/public/io/ring.h | 122 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+)