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

[Brian Woods <brian.woods@xilinx.com>]

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@xilinx.com> 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.

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

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.

Brian

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