| Message ID | 7cce89f4-962e-bfbe-7d30-18fea7515bed@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/p2m: type checking adjustments | expand |
On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <jbeulich@suse.com> 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@suse.com> > 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@suse.com/ -George
On 01.02.2024 14:32, George Dunlap wrote: > On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <jbeulich@suse.com> 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@suse.com> >> > > 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@suse.com/ 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. I can't tell what git doesn't like. The error messages (the one you see and the one I got) tell me nothing. 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.) Jan
On Thu, Feb 1, 2024 at 10:15 PM Jan Beulich <jbeulich@suse.com> wrote: > On 01.02.2024 14:32, George Dunlap wrote: > > On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <jbeulich@suse.com> 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@suse.com> > >> > > > > 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@suse.com/ > > 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
On 07.02.2024 04:07, George Dunlap wrote: > On Thu, Feb 1, 2024 at 10:15 PM Jan Beulich <jbeulich@suse.com> wrote: >> On 01.02.2024 14:32, George Dunlap wrote: >>> On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <jbeulich@suse.com> 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@suse.com> >>>> >>> >>> 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@suse.com/ >> >> 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
On 07.02.2024 04:07, George Dunlap wrote: > On Thu, Feb 1, 2024 at 10:15 PM Jan Beulich <jbeulich@suse.com> 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
--- 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; /* 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; } @@ -568,7 +568,7 @@ p2m_remove_entry(struct p2m_domain *p2m, for ( i = 0; i < (1UL << page_order); ++i ) { p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0, NULL, NULL); - if ( !p2m_is_hole(t) && !p2m_is_special(t) && !p2m_is_shared(t) ) + if ( !(p2m_is_hole(t) | p2m_is_special(t) | p2m_is_shared(t)) ) { set_gpfn_from_mfn(mfn_x(mfn) + i, gfn_x(gfn) + i); paging_mark_pfn_dirty(p2m->domain, _pfn(gfn_x(gfn) + i));
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@suse.com> --- RFC: The 3-way checks look to be a general problem for gcc, but even in some 2-way cases it doesn't manage to fold the expressions. Hence it's worth considering to go farther with this transformation, as long as the idea isn't disliked in general. --- v2: Re-base over change to earlier patch.