1. Patchew
  2. Xen
  3. View series

[PATCH] public: add RING_NR_UNCONSUMED_*() macros to ring.h

Juergen Gross posted 1 patch 2 years, 4 months ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20211126065547.22644-1-jgross@suse.com
There is a newer version of this series
xen/include/public/io/ring.h | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
[PATCH] public: add RING_NR_UNCONSUMED_*() macros to ring.h
Posted by Juergen Gross 2 years, 4 months ago 
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
Re: [PATCH] public: add RING_NR_UNCONSUMED_*() macros to ring.h
Posted by Jan Beulich 2 years, 4 months ago 
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
Re: [PATCH] public: add RING_NR_UNCONSUMED_*() macros to ring.h
Posted by Juergen Gross 2 years, 4 months ago 
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
Re: [PATCH] public: add RING_NR_UNCONSUMED_*() macros to ring.h
Posted by Jan Beulich 2 years, 4 months ago 
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
Re: [PATCH] public: add RING_NR_UNCONSUMED_*() macros to ring.h
Posted by Simon Kuenzer 2 years, 4 months ago 
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
>
Re: [PATCH] public: add RING_NR_UNCONSUMED_*() macros to ring.h
Posted by Manuel Bouyer 2 years, 4 months ago 
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

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--
Re: [PATCH] public: add RING_NR_UNCONSUMED_*() macros to ring.h
Posted by Durrant, Paul 2 years, 4 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.