Re: [Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap

[Julien Grall <julien.grall@xxxxxxx>]

Hi Brian,

On 17/10/2019 23:34, Brian Woods wrote:
> On Thu, Oct 17, 2019 at 10:49:15PM +0100, Julien Grall wrote:
> > On Thu, 17 Oct 2019 at 22:20, Brian Woods <brian.woods@xxxxxxxxxx> wrote:
> > > On Thu, Oct 17, 2019 at 09:34:51PM +0100, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > As a user such message would likely put me off. You tell me there are
> > > > an overlap, but you don't provide more information even if you likely
> > > > have the information in place. However...
> > > Well, I suppose the message could be changed to something like:
> > > "WARNING: overlap detected in the above memory module addresses."
> > > or something to more directly guide the users to the section.  Maybe
> > > move the 'printk("\n");' after the warning so it's grouped tighter with
> > > the module information.
> > My point stands even for this sort of message. You know the exact
> > overlap, so why would you hide it from the users?
> We're not hiding it.  You're not cluttering up the log with the same
> data multiple times.  See below.
While the values are the same, the data is printed in a different way to help 
the users.
> > > > ... What's wrong with just dumping the information here directly?
> > > IMO, it is better to have all the information printed in one spot.
> > > There is less to go through and easier to find out what is happening.
> > > There is also the fact that we do not have to print things twice (2 sets
> > > of names, starting addresses and ending addresses per overlap) when it
> > > is going to be printed in the near future anyway.  The cost of this is
> > > just one initdata bool, which while I am not thrilled about, does not
> > > seem that expensive (compared to a nested loop or printing out at least
> > > (16*2 + 12) * 2 characters per overlap(at least on Arm64)).
> > Again, this is boot code and not a path that is going to be called
> > hundreds of time. So performance is the last thing I care in this
> > patch.
> >
> > If we try to help the users by telling them there is an overlap
> > between modules, then we should do it properly and tell them the exact
> > overlap. Otherwise this is nearly as pointless as a crash later on in
> > the boot process.
> >
> > I also don't want a double for loop or any additional global variable
> > when it can be done by simply adding a check in add_boot_module().
> This isn't about performance (other than the nested for), this is about
> providing a relatively clean and sane log to read.  It's not that
> difficult to go through the addresses and see conflicts.  This also
> keeps it all in one part of the log and shorter without losing
> information.  Shorter and well structured logs (without losing info)
> makes it easier to read.  Making logs easier to read helps everyone.
>
> Showing the addresses and module name itself will take 2 lines assuming
> you stay within 80 chars.  (16*2 + 12) * 2 = 88, that's without spaces,
> '0x's or any sort of message explaining what's actually going wrong.
> The module names and addresses will be printed out anyway in the near
> future, so why not group them together?
Here again you argue about the performance and smaller message... This is a 
warning (so not printed in the normal course) and Xen is likely to break 
afterwards. So what you want here is a big fat warning and not a small one and 
easy to miss.
> The purpose of the warning is to tell the user something is wrong, both
> messages do that and provide the information to determine what's wrong
We both can probably figure out with more or less some effort. If you have 2-3 
modules that's fine. But if you have 10, then it is becoming more complex.
My time is quite valuable and therefore I want the hypervisor to help me find.

To make an analogy, would you like if your compiler tells you "There is a bug in 
file X" but does not tell you the exact line? I personally wouldn't, even if the 
file is fairly small.
So while I am happy to se a way to check the modules, I dislike this approach. 
So:

Nacked-by: Julien Grall <julien.grall@xxxxxxx>

Cheers,

--
Julien Grall

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