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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

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@xxxxxxxx> 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@xxxxxxxxxx>
> Cc: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
> Cc: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Cc: Paul Durrant <paul@xxxxxxx>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> 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
>