| Message ID | 20211126065547.22644-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | public: add RING_NR_UNCONSUMED_*() macros to ring.h | expand |
On 26.11.2021 07:55, Juergen Gross wrote: > Today RING_HAS_UNCONSUMED_*() macros are returning the number of > unconsumed requests or responses instead of a boolean as the name of > the macros would imply. > > As this "feature" is already being used, rename the macros to > RING_NR_UNCONSUMED_*() and define the RING_HAS_UNCONSUMED_*() macros > by using the new RING_NR_UNCONSUMED_*() macros. In order to avoid > future misuse let RING_HAS_UNCONSUMED_*() really return a boolean. I don't think we can go this far; consumers of our headers may choose to do so, but anyone taking the headers verbatim would be at risk of getting screwed if they had any instance of abuse in their trees. IOW I think the original-name macros ought to be direct aliases of the new-name ones, and only in Linux'es clone you could then go further. Jan
On 26.11.21 10:17, Jan Beulich wrote: > On 26.11.2021 07:55, Juergen Gross wrote: >> Today RING_HAS_UNCONSUMED_*() macros are returning the number of >> unconsumed requests or responses instead of a boolean as the name of >> the macros would imply. >> >> As this "feature" is already being used, rename the macros to >> RING_NR_UNCONSUMED_*() and define the RING_HAS_UNCONSUMED_*() macros >> by using the new RING_NR_UNCONSUMED_*() macros. In order to avoid >> future misuse let RING_HAS_UNCONSUMED_*() really return a boolean. > > I don't think we can go this far; consumers of our headers may choose > to do so, but anyone taking the headers verbatim would be at risk of > getting screwed if they had any instance of abuse in their trees. IOW > I think the original-name macros ought to be direct aliases of the > new-name ones, and only in Linux'es clone you could then go further. Fine with me. I'm inclined to add a comment hinting at that possibility then. Juergen
On 26.11.2021 10:21, Juergen Gross wrote: > On 26.11.21 10:17, Jan Beulich wrote: >> On 26.11.2021 07:55, Juergen Gross wrote: >>> Today RING_HAS_UNCONSUMED_*() macros are returning the number of >>> unconsumed requests or responses instead of a boolean as the name of >>> the macros would imply. >>> >>> As this "feature" is already being used, rename the macros to >>> RING_NR_UNCONSUMED_*() and define the RING_HAS_UNCONSUMED_*() macros >>> by using the new RING_NR_UNCONSUMED_*() macros. In order to avoid >>> future misuse let RING_HAS_UNCONSUMED_*() really return a boolean. >> >> I don't think we can go this far; consumers of our headers may choose >> to do so, but anyone taking the headers verbatim would be at risk of >> getting screwed if they had any instance of abuse in their trees. IOW >> I think the original-name macros ought to be direct aliases of the >> new-name ones, and only in Linux'es clone you could then go further. > > Fine with me. I'm inclined to add a comment hinting at that possibility > then. Indeed, please do. Jan
Hi Juergen, thanks a lot for putting us in CC. From the Unikraft perspective, we are fine with the change because we currently maintain a copy of the Xen headers in our tree. Our main reason is that we aim to keep compiling easier by avoiding off-tree references. Obviously, we have to update our copy but a diff will tell us where we need to adopt our code. In general, I like the clarity of the interface change that you are suggesting. Simon > On 26. Nov 2021, at 07:55, Juergen Gross <jgross@suse.com> wrote: > > Today RING_HAS_UNCONSUMED_*() macros are returning the number of > unconsumed requests or responses instead of a boolean as the name of > the macros would imply. > > As this "feature" is already being used, rename the macros to > RING_NR_UNCONSUMED_*() and define the RING_HAS_UNCONSUMED_*() macros > by using the new RING_NR_UNCONSUMED_*() macros. In order to avoid > future misuse let RING_HAS_UNCONSUMED_*() really return a boolean. > > Note that the known misuses need to be switched to the new > RING_NR_UNCONSUMED_*() macros when using this version of ring.h. > > Cc: Roger Pau Monne <roger.pau@citrix.com> > Cc: Manuel Bouyer <bouyer@antioche.eu.org> > Cc: Simon Kuenzer <simon.kuenzer@neclab.eu> > Cc: Paul Durrant <paul@xen.org> > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > I have checked Xen, Mini-OS, qemu, grub2, OVMF and Linux kernel for > misuses of the RING_HAS_UNCONSUMED_*() macros. There is currently only > one instance in the Linux kernel netback driver. The BSDs, UNIKRAFT > and Windows PV drivers should be checked for misuse, too. > --- > xen/include/public/io/ring.h | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h > index c486c457e0..efbc152174 100644 > --- a/xen/include/public/io/ring.h > +++ b/xen/include/public/io/ring.h > @@ -208,11 +208,11 @@ typedef struct __name##_back_ring __name##_back_ring_t > (RING_FREE_REQUESTS(_r) == 0) > > /* Test if there are outstanding messages to be processed on a ring. */ > -#define RING_HAS_UNCONSUMED_RESPONSES(_r) \ > +#define RING_NR_UNCONSUMED_RESPONSES(_r) \ > ((_r)->sring->rsp_prod - (_r)->rsp_cons) > > #ifdef __GNUC__ > -#define RING_HAS_UNCONSUMED_REQUESTS(_r) ({ \ > +#define RING_NR_UNCONSUMED_REQUESTS(_r) ({ \ > unsigned int req = (_r)->sring->req_prod - (_r)->req_cons; \ > unsigned int rsp = RING_SIZE(_r) - \ > ((_r)->req_cons - (_r)->rsp_prod_pvt); \ > @@ -220,13 +220,16 @@ typedef struct __name##_back_ring __name##_back_ring_t > }) > #else > /* Same as above, but without the nice GCC ({ ... }) syntax. */ > -#define RING_HAS_UNCONSUMED_REQUESTS(_r) \ > +#define RING_NR_UNCONSUMED_REQUESTS(_r) \ > ((((_r)->sring->req_prod - (_r)->req_cons) < \ > (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt))) ? \ > ((_r)->sring->req_prod - (_r)->req_cons) : \ > (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt))) > #endif > > +#define RING_HAS_UNCONSUMED_RESPONSES(_r) (!!RING_NR_UNCONSUMED_RESPONSES(_r)) > +#define RING_HAS_UNCONSUMED_REQUESTS(_r) (!!RING_NR_UNCONSUMED_REQUESTS(_r)) > + > /* Direct access to individual ring elements, by index. */ > #define RING_GET_REQUEST(_r, _idx) \ > (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) > -- > 2.26.2 >
On Fri, Nov 26, 2021 at 07:55:47AM +0100, Juergen Gross wrote: > Today RING_HAS_UNCONSUMED_*() macros are returning the number of > unconsumed requests or responses instead of a boolean as the name of > the macros would imply. > > As this "feature" is already being used, rename the macros to > RING_NR_UNCONSUMED_*() and define the RING_HAS_UNCONSUMED_*() macros > by using the new RING_NR_UNCONSUMED_*() macros. In order to avoid > future misuse let RING_HAS_UNCONSUMED_*() really return a boolean. > > Note that the known misuses need to be switched to the new > RING_NR_UNCONSUMED_*() macros when using this version of ring.h. AFAIK NetBSD is using RING_HAS_UNCONSUMED as a booleanm so it should be fine with this change
On 25/11/2021 22:55, Juergen Gross wrote: > Today RING_HAS_UNCONSUMED_*() macros are returning the number of > unconsumed requests or responses instead of a boolean as the name of > the macros would imply. > > As this "feature" is already being used, rename the macros to > RING_NR_UNCONSUMED_*() and define the RING_HAS_UNCONSUMED_*() macros > by using the new RING_NR_UNCONSUMED_*() macros. In order to avoid > future misuse let RING_HAS_UNCONSUMED_*() really return a boolean. > > Note that the known misuses need to be switched to the new > RING_NR_UNCONSUMED_*() macros when using this version of ring.h. > > Cc: Roger Pau Monne <roger.pau@citrix.com> > Cc: Manuel Bouyer <bouyer@antioche.eu.org> > Cc: Simon Kuenzer <simon.kuenzer@neclab.eu> > Cc: Paul Durrant <paul@xen.org> > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > I have checked Xen, Mini-OS, qemu, grub2, OVMF and Linux kernel for > misuses of the RING_HAS_UNCONSUMED_*() macros. There is currently only > one instance in the Linux kernel netback driver. The BSDs, UNIKRAFT > and Windows PV drivers should be checked for misuse, too. I don't think there will be any problem with Windows. Paul
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h index c486c457e0..efbc152174 100644 --- a/xen/include/public/io/ring.h +++ b/xen/include/public/io/ring.h @@ -208,11 +208,11 @@ typedef struct __name##_back_ring __name##_back_ring_t (RING_FREE_REQUESTS(_r) == 0) /* Test if there are outstanding messages to be processed on a ring. */ -#define RING_HAS_UNCONSUMED_RESPONSES(_r) \ +#define RING_NR_UNCONSUMED_RESPONSES(_r) \ ((_r)->sring->rsp_prod - (_r)->rsp_cons) #ifdef __GNUC__ -#define RING_HAS_UNCONSUMED_REQUESTS(_r) ({ \ +#define RING_NR_UNCONSUMED_REQUESTS(_r) ({ \ unsigned int req = (_r)->sring->req_prod - (_r)->req_cons; \ unsigned int rsp = RING_SIZE(_r) - \ ((_r)->req_cons - (_r)->rsp_prod_pvt); \ @@ -220,13 +220,16 @@ typedef struct __name##_back_ring __name##_back_ring_t }) #else /* Same as above, but without the nice GCC ({ ... }) syntax. */ -#define RING_HAS_UNCONSUMED_REQUESTS(_r) \ +#define RING_NR_UNCONSUMED_REQUESTS(_r) \ ((((_r)->sring->req_prod - (_r)->req_cons) < \ (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt))) ? \ ((_r)->sring->req_prod - (_r)->req_cons) : \ (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt))) #endif +#define RING_HAS_UNCONSUMED_RESPONSES(_r) (!!RING_NR_UNCONSUMED_RESPONSES(_r)) +#define RING_HAS_UNCONSUMED_REQUESTS(_r) (!!RING_NR_UNCONSUMED_REQUESTS(_r)) + /* Direct access to individual ring elements, by index. */ #define RING_GET_REQUEST(_r, _idx) \ (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
Today RING_HAS_UNCONSUMED_*() macros are returning the number of unconsumed requests or responses instead of a boolean as the name of the macros would imply. As this "feature" is already being used, rename the macros to RING_NR_UNCONSUMED_*() and define the RING_HAS_UNCONSUMED_*() macros by using the new RING_NR_UNCONSUMED_*() macros. In order to avoid future misuse let RING_HAS_UNCONSUMED_*() really return a boolean. Note that the known misuses need to be switched to the new RING_NR_UNCONSUMED_*() macros when using this version of ring.h. Cc: Roger Pau Monne <roger.pau@citrix.com> Cc: Manuel Bouyer <bouyer@antioche.eu.org> Cc: Simon Kuenzer <simon.kuenzer@neclab.eu> Cc: Paul Durrant <paul@xen.org> Signed-off-by: Juergen Gross <jgross@suse.com> --- I have checked Xen, Mini-OS, qemu, grub2, OVMF and Linux kernel for misuses of the RING_HAS_UNCONSUMED_*() macros. There is currently only one instance in the Linux kernel netback driver. The BSDs, UNIKRAFT and Windows PV drivers should be checked for misuse, too. --- xen/include/public/io/ring.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)