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_*() optionally really return a
boolean (can be activated by defining RING_HAS_UNCONSUMED_IS_BOOL).
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.
V2: make RING_HAS_UNCONSUMED_*() returning a bool optional (Jan Beulich)
---
xen/include/public/io/ring.h | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index c486c457e0..a7f492db39 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,25 @@ 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
+#ifdef RING_HAS_UNCONSUMED_IS_BOOL
+/*
+ * These variants should only be used in case no caller is abusing them for
+ * obtaining the number of unconsumed responses/requests.
+ */
+#define RING_HAS_UNCONSUMED_RESPONSES(_r) (!!RING_NR_UNCONSUMED_RESPONSES(_r))
+#define RING_HAS_UNCONSUMED_REQUESTS(_r) (!!RING_NR_UNCONSUMED_REQUESTS(_r))
+#else
+#define RING_HAS_UNCONSUMED_RESPONSES(_r) RING_NR_UNCONSUMED_RESPONSES(_r)
+#define RING_HAS_UNCONSUMED_REQUESTS(_r) RING_NR_UNCONSUMED_REQUESTS(_r)
+#endif
+
/* 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 08/12/2021 23:09, 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_*() optionally really return a > boolean (can be activated by defining RING_HAS_UNCONSUMED_IS_BOOL). > > 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. > V2: make RING_HAS_UNCONSUMED_*() returning a bool optional (Jan Beulich) > --- Reviewed-by: Paul Durrant <paul@xen.org>
On 09.12.2021 08:09, 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_*() optionally really return a
> boolean (can be activated by defining RING_HAS_UNCONSUMED_IS_BOOL).
>
> Note that the known misuses need to be switched to the new
> RING_NR_UNCONSUMED_*() macros when using this version of ring.h.
Is this last statement stale with the introduction of
RING_HAS_UNCONSUMED_IS_BOOL?
> --- 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); \
To answer the "whether" question this was likely good enough. I wonder
though whether the use of (_r)->sring->{rsp,req}_prod doesn't require
further sanitizing of the result as it's now intended to be used as a
count - afaict the returned values could easily be beyond the number of
ring elements when the other end is misbehaving. Or if not bounding
the values here, I would say the comment in context would need
updating / extending, to tell consumers that they may not blindly use
the returned values.
Also imo all new identifiers would better have a XEN_ prefix to avoid
further growing the risk of name space clashes. But I can see how this
would result in inconsistencies with existing names.
Jan
On 09.12.21 09:48, Jan Beulich wrote:
> On 09.12.2021 08:09, 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_*() optionally really return a
>> boolean (can be activated by defining RING_HAS_UNCONSUMED_IS_BOOL).
>>
>> Note that the known misuses need to be switched to the new
>> RING_NR_UNCONSUMED_*() macros when using this version of ring.h.
>
> Is this last statement stale with the introduction of
> RING_HAS_UNCONSUMED_IS_BOOL?
It should rather be modified like:
Note that the known misuses need to be switched to the new
RING_NR_UNCONSUMED_*() macros when using the RING_HAS_UNCONSUMED_*()
variants returning a boolean value.
>
>> --- 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); \
>
> To answer the "whether" question this was likely good enough. I wonder
> though whether the use of (_r)->sring->{rsp,req}_prod doesn't require
> further sanitizing of the result as it's now intended to be used as a
> count - afaict the returned values could easily be beyond the number of
> ring elements when the other end is misbehaving. Or if not bounding
> the values here, I would say the comment in context would need
> updating / extending, to tell consumers that they may not blindly use
> the returned values.
I'll modify the comment:
/*
* Test if there are outstanding messages to be processed on a ring.
* The answer is based on values writable by the other side, so further
* processing should fail gracefully in case the return value was wrong.
*/
> Also imo all new identifiers would better have a XEN_ prefix to avoid
> further growing the risk of name space clashes. But I can see how this
> would result in inconsistencies with existing names.
Yes, I do see the problem.
Would you support switching all the names to the XEN name space and
adding a section like:
#ifndef XEN_RING_NAMESPACE
/* Following for all macros etc. not in the XEN name space today */
#define x XEN_x
#endif
Juergen
On 09.12.2021 10:47, Juergen Gross wrote:
> On 09.12.21 09:48, Jan Beulich wrote:
>> On 09.12.2021 08:09, 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_*() optionally really return a
>>> boolean (can be activated by defining RING_HAS_UNCONSUMED_IS_BOOL).
>>>
>>> Note that the known misuses need to be switched to the new
>>> RING_NR_UNCONSUMED_*() macros when using this version of ring.h.
>>
>> Is this last statement stale with the introduction of
>> RING_HAS_UNCONSUMED_IS_BOOL?
>
> It should rather be modified like:
>
> Note that the known misuses need to be switched to the new
> RING_NR_UNCONSUMED_*() macros when using the RING_HAS_UNCONSUMED_*()
> variants returning a boolean value.
>
>>
>>> --- 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); \
>>
>> To answer the "whether" question this was likely good enough. I wonder
>> though whether the use of (_r)->sring->{rsp,req}_prod doesn't require
>> further sanitizing of the result as it's now intended to be used as a
>> count - afaict the returned values could easily be beyond the number of
>> ring elements when the other end is misbehaving. Or if not bounding
>> the values here, I would say the comment in context would need
>> updating / extending, to tell consumers that they may not blindly use
>> the returned values.
>
> I'll modify the comment:
>
> /*
> * Test if there are outstanding messages to be processed on a ring.
> * The answer is based on values writable by the other side, so further
> * processing should fail gracefully in case the return value was wrong.
> */
I'd recommend to avoid the word "fail" here. Maybe "... should deal
gracefully with the case ..."?
>> Also imo all new identifiers would better have a XEN_ prefix to avoid
>> further growing the risk of name space clashes. But I can see how this
>> would result in inconsistencies with existing names.
>
> Yes, I do see the problem.
>
> Would you support switching all the names to the XEN name space and
> adding a section like:
>
> #ifndef XEN_RING_NAMESPACE
> /* Following for all macros etc. not in the XEN name space today */
> #define x XEN_x
> #endif
Well, as that's not very neat either, I wouldn't go as far as saying
"support", but I certainly wouldn't object, and I also wouldn't mind
ack-ing such a change.
Jan
© 2016 - 2024 Red Hat, Inc.