* [PATCH v2] public: add RING_NR_UNCONSUMED_*() macros to ring.h
@ 2021-12-09 7:09 Juergen Gross
2021-12-09 7:50 ` Durrant, Paul
2021-12-09 8:48 ` Jan Beulich
0 siblings, 2 replies; 5+ messages in thread
From: Juergen Gross @ 2021-12-09 7:09 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Roger Pau Monne, Manuel Bouyer, Simon Kuenzer,
Paul Durrant
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] public: add RING_NR_UNCONSUMED_*() macros to ring.h
2021-12-09 7:09 [PATCH v2] public: add RING_NR_UNCONSUMED_*() macros to ring.h Juergen Gross
@ 2021-12-09 7:50 ` Durrant, Paul
2021-12-09 8:48 ` Jan Beulich
1 sibling, 0 replies; 5+ messages in thread
From: Durrant, Paul @ 2021-12-09 7:50 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: Roger Pau Monne, Manuel Bouyer, Simon Kuenzer, Paul Durrant
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] public: add RING_NR_UNCONSUMED_*() macros to ring.h
2021-12-09 7:09 [PATCH v2] public: add RING_NR_UNCONSUMED_*() macros to ring.h Juergen Gross
2021-12-09 7:50 ` Durrant, Paul
@ 2021-12-09 8:48 ` Jan Beulich
2021-12-09 9:47 ` Juergen Gross
1 sibling, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2021-12-09 8:48 UTC (permalink / raw)
To: Juergen Gross
Cc: Roger Pau Monne, Manuel Bouyer, Simon Kuenzer, Paul Durrant, xen-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] public: add RING_NR_UNCONSUMED_*() macros to ring.h
2021-12-09 8:48 ` Jan Beulich
@ 2021-12-09 9:47 ` Juergen Gross
2021-12-09 11:16 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Juergen Gross @ 2021-12-09 9:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Roger Pau Monne, Manuel Bouyer, Simon Kuenzer, Paul Durrant, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 3243 bytes --]
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
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] public: add RING_NR_UNCONSUMED_*() macros to ring.h
2021-12-09 9:47 ` Juergen Gross
@ 2021-12-09 11:16 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2021-12-09 11:16 UTC (permalink / raw)
To: Juergen Gross
Cc: Roger Pau Monne, Manuel Bouyer, Simon Kuenzer, Paul Durrant, xen-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-09 11:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-09 7:09 [PATCH v2] public: add RING_NR_UNCONSUMED_*() macros to ring.h Juergen Gross
2021-12-09 7:50 ` Durrant, Paul
2021-12-09 8:48 ` Jan Beulich
2021-12-09 9:47 ` Juergen Gross
2021-12-09 11:16 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.