Re: [PATCH v2 2/2] x86/p2m: aid the compiler in folding p2m_is_...()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2024 04:07, George Dunlap wrote:
> On Thu, Feb 1, 2024 at 10:15 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -379,7 +379,7 @@ struct page_info *p2m_get_page_from_gfn(
>              return page;
> =20
>          /* Error path: not a suitable GFN at all */
> -        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
> +        if ( !(p2m_is_ram(*t) | p2m_is_paging(*t) | p2m_is_pod(*t)) &&
>               !mem_sharing_is_fork(p2m->domain) )
>              return NULL;
>      }
> ```
> 
> Note the "=20" at the beginning of the empty line.  Why `patch` handles it
> but `git am` doesn't, who knows.
> 
> 
>> I'm also not aware of there
>> being a requirement that patches I send via email need to be
>> "git am"-able (unlike in xsa.git, where I edit patches enough to be
>> suitable for that), nor am I aware how I would convince my email
>> client and/or server to omit whatever git doesn't like or to add
>> whatever git is missing.
>>
>> Bottom line - your response would be actionable by me only in so far
>> as I could switch to using "git send-email". Which I'm afraid I'm not
>> going to do unless left with no other choice. The way I've been
>> sending patches has worked well for over 20 years, and for different
>> projects. (I'm aware Andrew has some special "Jan" command to apply
>> patches I send, but I don't know any specifics.)
>>
> 
> In the general case, I'm not going to review a patch without being able to
> see it in context; and it's not reasonable to expect reviewers to have
> specific contributor-specific scripts for doing so.  If we run into this
> issue in the future, and you want my review, you may have to post a git
> tree somewhere, or attach the patch as an attachment or something.  (Or you
> can try to figure out why `git am` isn't working and try to upstream a fix.)
> 
> That said, in this case, context isn't really necessary to understand the
> change, so it won't be necessary.
> 
> The logic of the change is obviously correct; but it definitely reduces the
> readability.  I kind of feel like whether this sort of optimization is
> worth the benefits is more a general x86 maintainer policy decision.  Maybe
> we can talk about it at the next maintainer's meeting I'll be at?

While you weren't able to be there, I brought this up nevertheless, and
both Andrew and Roger agreed with you. Therefore I'll drop this patch
and adjust the other one.

Jan