1. Patchew
  2. Xen
  3. View series

[PATCH] Argo: drop meaningless mfn_valid() check

Jan Beulich posted 1 patch 5 months, 1 week ago
Failed in applying to current master (apply log)
[PATCH] Argo: drop meaningless mfn_valid() check
Posted by Jan Beulich 5 months, 1 week ago 
Holding a valid struct page_info * in hands already means the referenced
MFN is valid; there's no need to check that again. Convert the checking
logic to a switch(), to help keeping the extra (and questionable) x86-
only check in somewhat tidy shape.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Initially I had this (with less code churn) as

#ifdef CONFIG_X86
    if ( p2mt == p2m_ram_logdirty )
        ret = -EAGAIN;
    else
#endif
    if ( (p2mt != p2m_ram_rw) ||
         !get_page_type(page, PGT_writable_page) )
        ret = -EINVAL;

But the "else" placement seemed too ugly to me. Otoh there better
wouldn't be any special casing of log-dirty here (and instead such a
page be converted, perhaps right in check_get_page_from_gfn() when
readonly=false), at which point the odd "else" would go away, and the
if() likely again be preferable over the switch().

--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -1421,15 +1421,24 @@ find_ring_mfn(struct domain *d, gfn_t gf
         return ret;
 
     *mfn = page_to_mfn(page);
-    if ( !mfn_valid(*mfn) )
-        ret = -EINVAL;
+
+    switch ( p2mt )
+    {
+    case p2m_ram_rw:
+        if ( !get_page_and_type(page, d, PGT_writable_page) )
+            ret = -EINVAL;
+        break;
+
 #ifdef CONFIG_X86
-    else if ( p2mt == p2m_ram_logdirty )
+    case p2m_ram_logdirty:
         ret = -EAGAIN;
+        break;
 #endif
-    else if ( (p2mt != p2m_ram_rw) ||
-              !get_page_and_type(page, d, PGT_writable_page) )
+
+    default:
         ret = -EINVAL;
+        break;
+    }
 
     put_page(page);
Ping: [PATCH] Argo: drop meaningless mfn_valid() check
Posted by Jan Beulich 4 months, 2 weeks ago 
Christopher,

On 27.11.2023 14:55, Jan Beulich wrote:
> Holding a valid struct page_info * in hands already means the referenced
> MFN is valid; there's no need to check that again. Convert the checking
> logic to a switch(), to help keeping the extra (and questionable) x86-
> only check in somewhat tidy shape.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

much like "Argo: don't obtain excess page references" (with which the one
here actually also conflicts), this one is awaiting your ack or otherwise.
Note that the other one has now been pending for quite a bit more than a
year. I hope the same isn't going to happen here ...

Thanks, Jan

> ---
> Initially I had this (with less code churn) as
> 
> #ifdef CONFIG_X86
>     if ( p2mt == p2m_ram_logdirty )
>         ret = -EAGAIN;
>     else
> #endif
>     if ( (p2mt != p2m_ram_rw) ||
>          !get_page_type(page, PGT_writable_page) )
>         ret = -EINVAL;
> 
> But the "else" placement seemed too ugly to me. Otoh there better
> wouldn't be any special casing of log-dirty here (and instead such a
> page be converted, perhaps right in check_get_page_from_gfn() when
> readonly=false), at which point the odd "else" would go away, and the
> if() likely again be preferable over the switch().
> 
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -1421,15 +1421,24 @@ find_ring_mfn(struct domain *d, gfn_t gf
>          return ret;
>  
>      *mfn = page_to_mfn(page);
> -    if ( !mfn_valid(*mfn) )
> -        ret = -EINVAL;
> +
> +    switch ( p2mt )
> +    {
> +    case p2m_ram_rw:
> +        if ( !get_page_and_type(page, d, PGT_writable_page) )
> +            ret = -EINVAL;
> +        break;
> +
>  #ifdef CONFIG_X86
> -    else if ( p2mt == p2m_ram_logdirty )
> +    case p2m_ram_logdirty:
>          ret = -EAGAIN;
> +        break;
>  #endif
> -    else if ( (p2mt != p2m_ram_rw) ||
> -              !get_page_and_type(page, d, PGT_writable_page) )
> +
> +    default:
>          ret = -EINVAL;
> +        break;
> +    }
>  
>      put_page(page);
>  
>
Re: Ping: [PATCH] Argo: drop meaningless mfn_valid() check
Posted by Christopher Clark 4 months, 1 week ago 
On Sun, Dec 17, 2023 at 11:55 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> Christopher,
>
> On 27.11.2023 14:55, Jan Beulich wrote:
> > Holding a valid struct page_info * in hands already means the referenced
> > MFN is valid; there's no need to check that again. Convert the checking
> > logic to a switch(), to help keeping the extra (and questionable) x86-
> > only check in somewhat tidy shape.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com>

>
> much like "Argo: don't obtain excess page references" (with which the one
> here actually also conflicts), this one is awaiting your ack or otherwise.
> Note that the other one has now been pending for quite a bit more than a
> year. I hope the same isn't going to happen here ...

Thanks for your patience and the reminders, appreciated and this patch
can be applied.

Christopher


>
> Thanks, Jan
>
> > ---
> > Initially I had this (with less code churn) as
> >
> > #ifdef CONFIG_X86
> >     if ( p2mt == p2m_ram_logdirty )
> >         ret = -EAGAIN;
> >     else
> > #endif
> >     if ( (p2mt != p2m_ram_rw) ||
> >          !get_page_type(page, PGT_writable_page) )
> >         ret = -EINVAL;
> >
> > But the "else" placement seemed too ugly to me. Otoh there better
> > wouldn't be any special casing of log-dirty here (and instead such a
> > page be converted, perhaps right in check_get_page_from_gfn() when
> > readonly=false), at which point the odd "else" would go away, and the
> > if() likely again be preferable over the switch().
> >
> > --- a/xen/common/argo.c
> > +++ b/xen/common/argo.c
> > @@ -1421,15 +1421,24 @@ find_ring_mfn(struct domain *d, gfn_t gf
> >          return ret;
> >
> >      *mfn = page_to_mfn(page);
> > -    if ( !mfn_valid(*mfn) )
> > -        ret = -EINVAL;
> > +
> > +    switch ( p2mt )
> > +    {
> > +    case p2m_ram_rw:
> > +        if ( !get_page_and_type(page, d, PGT_writable_page) )
> > +            ret = -EINVAL;
> > +        break;
> > +
> >  #ifdef CONFIG_X86
> > -    else if ( p2mt == p2m_ram_logdirty )
> > +    case p2m_ram_logdirty:
> >          ret = -EAGAIN;
> > +        break;
> >  #endif
> > -    else if ( (p2mt != p2m_ram_rw) ||
> > -              !get_page_and_type(page, d, PGT_writable_page) )
> > +
> > +    default:
> >          ret = -EINVAL;
> > +        break;
> > +    }
> >
> >      put_page(page);
> >
> >
>
Re: Ping: [PATCH] Argo: drop meaningless mfn_valid() check
Posted by Christopher Clark 4 months, 1 week ago 
On Sat, Dec 23, 2023 at 12:47 PM Christopher Clark
<christopher.w.clark@gmail.com> wrote:
>
> On Sun, Dec 17, 2023 at 11:55 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > Christopher,
> >
> > On 27.11.2023 14:55, Jan Beulich wrote:
> > > Holding a valid struct page_info * in hands already means the referenced
> > > MFN is valid; there's no need to check that again. Convert the checking
> > > logic to a switch(), to help keeping the extra (and questionable) x86-
> > > only check in somewhat tidy shape.
> > >
> > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com>

I'd like to hold off on this just yet, sorry -- the change does look
ok as far as a transform being applied the prior logic and the
necessity of the check, but with it applied, it's not obvious that it
handles all the page types as best that it could there, so I'd like to
look at this (and the previously submitted patch again) please.

Christopher
Re: Ping: [PATCH] Argo: drop meaningless mfn_valid() check
Posted by Jan Beulich 4 months ago 
On 23.12.2023 22:35, Christopher Clark wrote:
> On Sat, Dec 23, 2023 at 12:47 PM Christopher Clark
> <christopher.w.clark@gmail.com> wrote:
>>
>> On Sun, Dec 17, 2023 at 11:55 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> Christopher,
>>>
>>> On 27.11.2023 14:55, Jan Beulich wrote:
>>>> Holding a valid struct page_info * in hands already means the referenced
>>>> MFN is valid; there's no need to check that again. Convert the checking
>>>> logic to a switch(), to help keeping the extra (and questionable) x86-
>>>> only check in somewhat tidy shape.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com>
> 
> I'd like to hold off on this just yet, sorry -- the change does look
> ok as far as a transform being applied the prior logic and the
> necessity of the check, but with it applied, it's not obvious that it
> handles all the page types as best that it could there, so I'd like to
> look at this (and the previously submitted patch again) please.

I'm puzzled: This patch merely removes a pointless check. Whatever is
lacking with it in place will have been lacking before. Also can you
please give a rough estimate towards when you'll be getting back on
this, or ideally on both patches?

Jan
Ping: Ping: [PATCH] Argo: drop meaningless mfn_valid() check
Posted by Jan Beulich 3 months ago 
On 04.01.2024 09:12, Jan Beulich wrote:
> On 23.12.2023 22:35, Christopher Clark wrote:
>> On Sat, Dec 23, 2023 at 12:47 PM Christopher Clark
>> <christopher.w.clark@gmail.com> wrote:
>>>
>>> On Sun, Dec 17, 2023 at 11:55 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> Christopher,
>>>>
>>>> On 27.11.2023 14:55, Jan Beulich wrote:
>>>>> Holding a valid struct page_info * in hands already means the referenced
>>>>> MFN is valid; there's no need to check that again. Convert the checking
>>>>> logic to a switch(), to help keeping the extra (and questionable) x86-
>>>>> only check in somewhat tidy shape.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com>
>>
>> I'd like to hold off on this just yet, sorry -- the change does look
>> ok as far as a transform being applied the prior logic and the
>> necessity of the check, but with it applied, it's not obvious that it
>> handles all the page types as best that it could there, so I'd like to
>> look at this (and the previously submitted patch again) please.
> 
> I'm puzzled: This patch merely removes a pointless check. Whatever is
> lacking with it in place will have been lacking before. Also can you
> please give a rough estimate towards when you'll be getting back on
> this, or ideally on both patches?

Another month later: I'll give it this week, and without hearing back
I'll commit what there is some time next week, with the R-b you provided.

Jan
Re: Ping: Ping: [PATCH] Argo: drop meaningless mfn_valid() check
Posted by Christopher Clark 2 months, 4 weeks ago 
On Mon, Feb 5, 2024 at 4:17 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.01.2024 09:12, Jan Beulich wrote:
> > On 23.12.2023 22:35, Christopher Clark wrote:
> >> On Sat, Dec 23, 2023 at 12:47 PM Christopher Clark
> >> <christopher.w.clark@gmail.com> wrote:
> >>>
> >>> On Sun, Dec 17, 2023 at 11:55 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> Christopher,
> >>>>
> >>>> On 27.11.2023 14:55, Jan Beulich wrote:
> >>>>> Holding a valid struct page_info * in hands already means the referenced
> >>>>> MFN is valid; there's no need to check that again. Convert the checking
> >>>>> logic to a switch(), to help keeping the extra (and questionable) x86-
> >>>>> only check in somewhat tidy shape.
> >>>>>
> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>
> >>> Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com>
> >>
> >> I'd like to hold off on this just yet, sorry -- the change does look
> >> ok as far as a transform being applied the prior logic and the
> >> necessity of the check, but with it applied, it's not obvious that it
> >> handles all the page types as best that it could there, so I'd like to
> >> look at this (and the previously submitted patch again) please.
> >
> > I'm puzzled: This patch merely removes a pointless check. Whatever is
> > lacking with it in place will have been lacking before. Also can you
> > please give a rough estimate towards when you'll be getting back on
> > this, or ideally on both patches?
>
> Another month later: I'll give it this week, and without hearing back
> I'll commit what there is some time next week, with the R-b you provided.

Sorry for the slow communication. I'm not unaware of this but it has
been difficult to provide an update on when I will be able to give it
the appropriate attention.
I understand that you'd appreciate a response before next week, so
I'll aim for that.

thanks,

Christopher

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.