[v5,10/18] x86/mem_sharing: Convert MEM_SHARING_DESTROY_GFN to a bool

Commit Message

Tamas K Lengyel Jan. 21, 2020, 5:49 p.m. UTC

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 <[e-mail]>
Reviewed-by: Jan Beulich <[e-mail]>
---
 xen/arch/x86/mm/mem_sharing.c     | 9 ++++-----
 xen/include/asm-x86/mem_sharing.h | 5 ++---
 2 files changed, 6 insertions(+), 8 deletions(-)

Comments

George Dunlap Jan. 23, 2020, 4:14 p.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Jan Beulich <[e-mail]>

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.)

 -George

Tamas K Lengyel Jan. 23, 2020, 4:23 p.m. UTC | #2

On Thu, Jan 23, 2020 at 9:14 AM George Dunlap <[e-mail]> 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 <[e-mail]>
> > Reviewed-by: Jan Beulich <[e-mail]>
>
> 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.

Tamas

George Dunlap Jan. 23, 2020, 4:32 p.m. UTC | #3

On 1/23/20 4:23 PM, Tamas K Lengyel wrote:
> On Thu, Jan 23, 2020 at 9:14 AM George Dunlap <[e-mail]> 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 <[e-mail]>
>>> Reviewed-by: Jan Beulich <[e-mail]>
>>
>> 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.

 -George

Jan Beulich Jan. 23, 2020, 4:37 p.m. UTC | #4

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 <[e-mail]> 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 <[e-mail]>
>>>> Reviewed-by: Jan Beulich <[e-mail]>
>>>
>>> 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

George Dunlap Jan. 23, 2020, 4:42 p.m. UTC | #5

On 1/23/20 4:37 PM, Jan Beulich wrote:
> 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 <[e-mail]> 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 <[e-mail]>
>>>>> Reviewed-by: Jan Beulich <[e-mail]>
>>>>
>>>> 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.

I don't think I'm co-maintainer of this; and you're a less-specific
maintainer than Tamas, right?  Do you need my Ack?

I'm happy to go back and Ack all the mm/ ones you've given an R-b to up
until this point in the series; I just didn't think it was necessary.

I'll probably Ack patch 2 once I become convinced the change in that
patch is necessary (which I'm guessing might be when I get to 15/18).

 -George

Jan Beulich Jan. 23, 2020, 4:55 p.m. UTC | #6

On 23.01.2020 17:42, George Dunlap wrote:
> On 1/23/20 4:37 PM, Jan Beulich wrote:
>> 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 <[e-mail]> 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 <[e-mail]>
>>>>>> Reviewed-by: Jan Beulich <[e-mail]>
>>>>>
>>>>> 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.
> 
> I don't think I'm co-maintainer of this; and you're a less-specific
> maintainer than Tamas, right?  Do you need my Ack?

Well, I was under the impression that the maintainership nesting
was hierarchical, i.e. the next level up would become the relevant
one in cases like this one. But I'm of course happy to commit the
parts of this series which are ready, if just any less specific
maintainer's ack is sufficient. Probably tomorrow morning then.

Jan

Patch

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 5d840550f4..da7d142ad8 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1170,7 +1170,7 @@  err_out:
  */
 int __mem_sharing_unshare_page(struct domain *d,
                                unsigned long gfn,
-                               uint16_t flags)
+                               bool destroy)
 {
     p2m_type_t p2mt;
     mfn_t mfn;
@@ -1226,7 +1226,7 @@  int __mem_sharing_unshare_page(struct domain *d,
      * If the GFN is getting destroyed drop the references to MFN
      * (possibly freeing the page), and exit early.
      */
-    if ( flags & MEM_SHARING_DESTROY_GFN )
+    if ( destroy )
     {
         if ( !last_gfn )
             mem_sharing_gfn_destroy(page, d, gfn_info);
@@ -1317,9 +1317,8 @@  int relinquish_shared_pages(struct domain *d)
         mfn = p2m->get_entry(p2m, _gfn(gfn), &t, &a, 0, NULL, NULL);
         if ( mfn_valid(mfn) && p2m_is_shared(t) )
         {
-            /* Does not fail with ENOMEM given the DESTROY flag */
-            BUG_ON(__mem_sharing_unshare_page(
-                       d, gfn, MEM_SHARING_DESTROY_GFN));
+            /* Does not fail with ENOMEM given "destroy" is set to true */
+            BUG_ON(__mem_sharing_unshare_page(d, gfn, true));
             /*
              * Clear out the p2m entry so no one else may try to
              * unshare.  Must succeed: we just read the old entry and
diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h
index a10af9d570..53760a2896 100644
--- a/xen/include/asm-x86/mem_sharing.h
+++ b/xen/include/asm-x86/mem_sharing.h
@@ -75,16 +75,15 @@  struct page_sharing_info
 unsigned int mem_sharing_get_nr_saved_mfns(void);
 unsigned int mem_sharing_get_nr_shared_mfns(void);
 
-#define MEM_SHARING_DESTROY_GFN       (1<<1)
 /* Only fails with -ENOMEM. Enforce it with a BUG_ON wrapper. */
 int __mem_sharing_unshare_page(struct domain *d,
                                unsigned long gfn,
-                               uint16_t flags);
+                               bool destroy);
 
 static inline int mem_sharing_unshare_page(struct domain *d,
                                            unsigned long gfn)
 {
-    int rc = __mem_sharing_unshare_page(d, gfn, 0);
+    int rc = __mem_sharing_unshare_page(d, gfn, false);
     BUG_ON(rc && (rc != -ENOMEM));
     return rc;
 }