[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Juergen Gross <jgross@xxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 9 Dec 2021 12:16:32 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=BcEOLFgAyG+3D5ohtnijoJ1iOU8F97z6QoWePlLNKRw=; b=dKXZOD1pUGQcCA8M4SZdjw1e4zkXEgB+r4Wb51qSTJkTt7Goh5q3n66FFqgY3VP5HBuageXqUCsNBw9eqaOMUUQ5Y0bXHLYU7ZnAc+Q1C3A0+xHHYSMmMYlLkMhCvo2wL7pp63ep1GzmbKdytXsuy13O3WAzv4o8GmUmDR/oQLE8G0v3tCCF5fZCJHV0sxAibjSYH4AGxczfUT1FHzwKfF+Mit3Ap/6I+tm3jL8quI5c1bLsHCQmm7LbFgy7lxY364YCRIZQALdjjfDxAfkwwjUV/sx/5F6KtscecJQoV7N2Bjmkqs8L0+7cg7pArLtgdn6dr5G3YFd2ybzSq+aZlQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=gUelqgg1d5d/PePmqDmJM2nVMDlM3C6dTq+F+sQwss8Pgz+NY/+7vkYT0rjog4Uk7DnghKuh6tBSsCpaiPol4VgWRCngpSYrhv+E1UV+vgUkOvUH70X/V1PPIZctcraB+Nx+5cldgJ7UV3VrEeWn2rOeh6Gq+juyVF1uzPVeXpZzyBWG/pItByz+WJMyrtgOLQz+Aj6tgQm7GCUHyQ2liUfNnM/8J273lkbgbpMCsOW36Xa/qEEnfd2SNK22T7roKMpo6QKRwMledMIHa6VJ+q/tpaE073sCewGAdOGraHHxizvgAyujKIEW8cTcN0t2dsIpysubYQzSn6sJDtFPlw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>, Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>, Simon Kuenzer <simon.kuenzer@xxxxxxxxx>, Paul Durrant <paul@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 09 Dec 2021 11:16:50 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.