Hi Tom,
On Wed, 6 Sept 2023 at 11:58, Tom Rini <tr...@konsulko.com> wrote:
>
> On Tue, Sep 05, 2023 at 12:16:56PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Sun, 3 Sept 2023 at 13:25, Tom Rini <tr...@konsulko.com> wrote:
> > >
> > > On Sun, Sep 03, 2023 at 12:06:13PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Sun, 3 Sept 2023 at 10:42, Tom Rini <tr...@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Aug 31, 2023 at 06:15:19PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Wed, 23 Aug 2023 at 09:14, Tom Rini <tr...@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Wed, Aug 23, 2023 at 07:42:03AM -0600, Simon Glass wrote:
> > > > > > >
> > > > > > > > When lmb runs out of space in its internal tables, it gives
> > errors on
> > > > > > > > every fs load operation. This is horribly confusing, as the
> > poor user
> > > > > > > > tries different memory regions one at a time.
> > > > > > > >
> > > > > > > > Use the updated API to check the error code and print a helpful
> > message.
> > > > > > > > Also, allow the operation to proceed.
> > > > > > > >
> > > > > > > > Update the tests accordingly.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org>
> > > > > > > [snip]
> > > > > > > > + if (ret == -E2BIG) {
> > > > > > > > + log_warning("Reservation tables exhausted: see
> > CONFIG_LMB_USE_MAX_REGIONS\n");
> > > > > > > > + return 0;
> > > > > > > > + }
> > > > > > >
> > > > > > > This isn't the right option. Everyone has
> > CONFIG_LMB_USE_MAX_REGIONS
> > > > > > > set. You would want to increase CONFIG_LMB_MAX_REGIONS.
> > > > > > >
> > > > > > > But it sounds like what you've found and fixed is an underlying
> > problem
> > > > > > > as to why 16 regions isn't enough in some common cases now? So
> > we could
> > > > > >
> > > > > > I don't think I have fixed anything. But I'll send v2 and perhaps it
> > > > > > will be clearer what is going on here.
> > > > > >
> > > > > > > possibly avoid the string size growth here and have a comment
> > that also
> > > > > > > matches up with the help under LMB_MAX_REGIONS?
> > > > > >
> > > > > > I don't know, sorry. The size of struct(lmb) on 64-bit sandbox is
> > > > > > about 400 bytes. There seems to be a lot of code to save not much
> > > > > > memory.
> > > > >
> > > > > What do you mean here? The alternative is not unlimited ranges but
> > > > > instead to define the limit of memory regions and limit of reserved
> > > > > ranges.
> > > >
> > > > A better alternative would be not to limit the number of regions, IMO,
> > > > i.e. have an array of regions that can grow as needed.
> > >
> > > That's not something that we have in the code today, however.
> >
> > No, I do have an arraylist thing that I plan to upstream, which could
> > handle that.
> >
> > But for this series, what do you think of the memregion idea? I am still
> > unsure of the saming we should use - see Heinrich's comments too.
>
> My biggest worry here is that we're papering over a real issue, and
> should either dig at that and see what's going on (should something be
> coalescing adjacent allocations? Were many platforms just right at the
> previous limit?) or just bump the default from 16 to "64 if EFI_LOADER"
> and then see if anything else really needs tweaking / cleaning up in the
> code itself. I know Heinrich has previously said the LMB system could
> be better (or something to that effect, I'm going from memory, sorry if
> I'm mis-stating things) and I don't object to replacing what we have
> with something more robust/smarter/etc.
I am not sure...my series was designed to rename the code to reduce
confusion, and print a useful message when we run out of regions. It
does not paper over the problem, but actually makes it more visible.
Regards,
Simon
