| Message ID | 580c6c48-9dd5-4296-8696-2b40beac2bc3@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Argo: drop meaningless mfn_valid() check | expand |
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); > >
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); > > > > >
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
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
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
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
--- 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);
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().