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

[George Dunlap <dunlapg@xxxxxxxxx>]

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.

 

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?

 -George