xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: David Woodhouse <dwmw2@infradead.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jeff Kubascik <jeff.kubascik@dornerworks.com>,
	Stewart Hildebrand <stewart.hildebrand@dornerworks.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits
Date: Thu, 19 Mar 2020 12:59:12 +0100	[thread overview]
Message-ID: <bd907f89-5366-23d7-96a3-d1146c862381@suse.com> (raw)
In-Reply-To: <4a1504117e7422a685ed2fec2b97bb15a0e1bf29.camel@infradead.org>

On 19.03.2020 11:26, David Woodhouse wrote:
> On Thu, 2020-03-19 at 09:49 +0100, Jan Beulich wrote:
>> On 18.03.2020 18:13, David Woodhouse wrote:
>>> The -MP makefile patch I posted yesterday... I almost didn't bother.
>>> And when I allowed myself to be talked into it, I was entirely
>>> unsurprised when a review came in basically asking me to prove a
>>> negative before the patch could proceed. So as far as I can tell, it'll
>>> fall by the wayside and the build will remain broken any time anyone
>>> removes or renames a header file. Because life's too short to invest
>>> the energy to make improvements like that.
>>
>> So are you saying that as a maintainer I should let go uncommented a
>> change which I'm unconvinced doesn't have negative side effects,
>> besides its positive intended behavioral change? The more that here
>> the workaround is rather trivial? As you may imagine, I've run into
>> the situation myself a number of times, without considering this a
>> reason to make any adjustments to the build machinery.
> 
> Jan, I would respectfully request that you take another look at your
> initial response, but put yourself in the shoes of a patch submitter:
> https://lists.xenproject.org/archives/html/xen-devel/2020-03/msg01171.html
> 
> You mention a "simple" workaround... but the workaround I've been using
> is to manually remove the offending .o.d files, one at a time (or at
> least one directory at a time), until the broken build starts working
> again. Is that what you meant? And you really didn't ever consider that
> it should be fixed?

No, the workaround I've been using (after initially doing the expensive
one you describe) was to simply put in an empty file (or perhaps one
with an #error directive) in the place of the deleted one, rebuild, and
all .*.o.d files would have been updated. I might do so even before
fully deleting the file.

> And the substance of the response is basically saying "this is voodoo
> and we can't touch it or unspecified things might break, but I have no
> idea where to tell you to look."
> 
> Looking back I realise that the concern about phony rules overriding
> pattern rules didn't even come from you; your concern was more nebulous
> and unaddressable. It looks like I came up with a straw man and shot
> *that* down in my later analysis (although that wasn't my intent; I
> think the concern about pattern rules really did come from somewhere).
> 
> You asked a question about "why isn't this default behaviour", which is
> kind of a silly question when asking about an option (-MP) that was
> added to GCC almost a decade after the initial -MD behaviour was
> established. Of *course* they didn't retroactively change the default.

I don't see at all why this would be "of course" - if there really
was no undue side effect, why couldn't they?

> Read that message again from the point of view of a contributor.
> Pretend it isn't even me; pretend it's someone attempting to make their
> first, trivial, improvement to the Xen ecosystem.
> 
> I hope you'll understand why my initial reaction was just a
> monosyllabic 'no'.

To be honest- no, I don't. I didn't say "no way". Instead I asked
back to see whether there's more background to this. It is a useful
piece of information to know that -MP post-dates -MD by 10 or more
years. It's still speculation of why a new option was added rather
than making this default behavior, but I feel less afraid of the
change this way than by an implied "this not going to do any harm"
without really being certain why there is a separate option in the
first place (and gcc doc also not saying anything to this effect).

I can certainly follow your sentiment, not the least because
especially in my early days I also frequently got back replies I
didn't like, in various projects. Yet in a case like this one I'm
afraid it is not the reviewer's job to point out the unsafety of
a change, but it's the submitter who has to (sufficiently) prove
that a change won't break anything. Yes, in the typical case,
when there's a recognizable bug, the reviewer would point this
out. But there are cases where there's no obvious bug, but
experience (and, as so often, insufficient documentation) tells
one to be wary of changes of, possibly, any kind.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

  reply	other threads:[~2020-03-19 11:59 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-04 15:14 [Xen-devel] [XEN PATCH v2 1/2] Check zone before merging adjacent blocks in heap Stewart Hildebrand
2020-02-04 15:14 ` [Xen-devel] [DO NOT APPLY XEN PATCH v2 2/2] Test case for buddy allocator merging issue Stewart Hildebrand
2020-02-04 15:22 ` [Xen-devel] [XEN PATCH v2 1/2] Check zone before merging adjacent blocks in heap Jan Beulich
2020-02-04 15:37   ` George Dunlap
2020-02-05  9:50     ` David Woodhouse
2020-02-05 10:02       ` Jan Beulich
2020-02-05 10:24         ` David Woodhouse
2020-02-05 10:49           ` Jan Beulich
2020-02-05 11:23             ` David Woodhouse
2020-02-05 13:37               ` Jan Beulich
2020-02-05 14:12                 ` David Woodhouse
2020-02-07 15:49                   ` David Woodhouse
2020-02-07 15:57                     ` [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits David Woodhouse
2020-02-07 20:27                       ` Julien Grall
2020-02-09 13:22                         ` David Woodhouse
2020-02-09 17:59                           ` Julien Grall
2020-03-17 21:39                         ` David Woodhouse
2020-02-20 11:10                       ` Jan Beulich
2020-03-17 21:52                         ` David Woodhouse
2020-03-18  9:56                           ` Jan Beulich
2020-03-18 12:31                             ` Julien Grall
2020-03-18 13:23                               ` Jan Beulich
2020-03-18 17:13                               ` David Woodhouse
2020-03-19  8:49                                 ` Jan Beulich
2020-03-19 10:26                                   ` David Woodhouse
2020-03-19 11:59                                     ` Jan Beulich [this message]
2020-03-19 13:54                                       ` David Woodhouse
2020-03-19 14:46                                         ` Jan Beulich
2020-02-07 15:57                     ` [Xen-devel] [PATCH 2/2] xen/mm: Introduce PG_state_uninitialised David Woodhouse
2020-02-07 16:30                       ` Xia, Hongyan
2020-02-07 16:32                         ` David Woodhouse
2020-02-07 16:40                           ` Xia, Hongyan
2020-02-07 17:06                             ` David Woodhouse
2020-02-07 18:04                               ` David Woodhouse
2020-02-20 11:59                                 ` Jan Beulich
2020-02-20 13:27                                   ` Julien Grall
2020-03-17 22:15                                   ` David Woodhouse
2020-03-18  8:53                                     ` Paul Durrant
2020-03-18 10:10                                       ` Jan Beulich
2020-03-18 10:41                                         ` Paul Durrant
2020-03-18 11:12                                           ` Jan Beulich
2020-03-18 10:03                                     ` Jan Beulich
2020-03-18 12:11                                       ` David Woodhouse
2020-03-18 13:27                                         ` Jan Beulich
2020-02-05 10:22       ` [Xen-devel] [XEN PATCH v2 1/2] Check zone before merging adjacent blocks in heap Julien Grall
2020-02-05 10:32         ` David Woodhouse
2020-02-05 11:36         ` David Woodhouse
2020-02-04 15:37   ` Stewart Hildebrand
2020-03-19 21:17 [Xen-devel] [PATCH 0/2] Handle David Woodhouse
2020-03-19 21:21 ` [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits David Woodhouse
2020-03-20 13:17   ` Paul Durrant

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bd907f89-5366-23d7-96a3-d1146c862381@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=dwmw2@infradead.org \
    --cc=george.dunlap@citrix.com \
    --cc=george.dunlap@eu.citrix.com \
    --cc=ian.jackson@eu.citrix.com \
    --cc=jeff.kubascik@dornerworks.com \
    --cc=julien@xen.org \
    --cc=konrad.wilk@oracle.com \
    --cc=sstabellini@kernel.org \
    --cc=stewart.hildebrand@dornerworks.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).