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:
>> On 01.02.2024 14:32, George Dunlap wrote:
>>> On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>
>>>> By using | instead of || or (in the negated form) && chances increase
>>>> for the compiler to recognize that both predicates can actually be
>>>> folded into an expression requiring just a single branch (via OR-ing
>>>> together the respective P2M_*_TYPES constants).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>
>>>
>>> Sorry for the delay.  Git complains that this patch is malformed:
>>>
>>> error: `git apply --index`: error: corrupt patch at line 28
>>>
>>> Similar complaint from patchew when it was posted:
>>>
>>> https://patchew.org/Xen/5d6c927e-7d7c-5754-e7eb-65d1e70f6222@xxxxxxxx/
>>
>> Not sure what to say. The patch surely is well-formed. It applies fine
>> using patch (when not taken from email). When taken from email, patch
>> mentions that it strips CRs (I'm running my email client on Windows),
>> but the saved email still applies fine. "git am" indeed is unhappy
>> when taking the plain file as saved from email, albeit here with an
>> error different from yours. If I edit the saved email to retain just
>> the From: and Subject: tags, all is fine.
>>
> 
> That still doesn't work for me.
> 
> 
>> I can't tell what git doesn't like. The error messages (the one you
>> see and the one I got) tell me nothing.
> 
> 
> The raw email looks like this:
> 
> ```
> --- 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.

Hmm. Nothing like that seen when I save that mail. Plus I recall having
an issue with this when applying patches coming from Shawn, where those
=20 got in the way, but only if I pruned the saved email before handing
to "git am".

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

Based on my own observation mentioned above, I assume "git am" is capable
of dealing with the =20, provided some specific further encoding
specification is present in the mail. Which I'd then have to assume is
missing from what Thunderbird sends, or the =20 is being introduced
without Thunderbird being involved.

> 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?

I see no problem doing so.

Jan