Re: [Xen-devel] [PATCH v5 10/18] x86/mem_sharing: Convert MEM_SHARING_DESTROY_GFN to a bool

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2020 17:32, George Dunlap wrote:
> On 1/23/20 4:23 PM, Tamas K Lengyel wrote:
>> On Thu, Jan 23, 2020 at 9:14 AM George Dunlap <george.dunlap@xxxxxxxxxx> 
>> wrote:
>>>
>>> On 1/21/20 5:49 PM, Tamas K Lengyel wrote:
>>>> MEM_SHARING_DESTROY_GFN is used on the 'flags' bitfield during unsharing.
>>>> However, the bitfield is not used for anything else, so just convert it to 
>>>> a
>>>> bool instead.
>>>>
>>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> But is there a particular advantage to getting rid of the bitfield?
>>>
>>> You're the maintainer, so it's your decision of course.  But if it were
>>> me I'd leave it as a bitfield so that all the bitfield code is there if
>>> it's needed in the future.  Flipping it to a bool, with the risk of
>>> flipping it back to a bitfield later, seems like pointless churn to me.
>>>
>>> (Although perhaps the reason will become evident by the time I get to
>>> the end of the series.)
>>
>> Provided its been many years since this code has been added with no
>> need for any extra bits, and with no expectations that this would
>> change any time soon, I wouldn't worry about that. True, there is very
>> little difference between keeping the code as-is vs converting it to
>> bool, but IMHO it makes the code easier to follow without you
>> wondering what might be those non-existent situations that warranted
>> it to be a bitmap to begin with.
> 
> It's definitely a judgement call, and I can see where you're coming
> from.  Like I said, if it were me I'd leave it, but it's not. :-)   Just
> wanted to raise the issue as I was going through.  I'd Ack it but you've
> already got an R-b.

Until your proposal gets accepted, isn't it that your ack is needed
despite the R-b? This is also why e.g. for patch 2 I didn't see a
point in sending any R-b, as the patch will need your ack anyway,
and it's so simple that "reviewed" would be an overstatement.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel