On Wed, Sep 03, 2025 at 09:41:18AM -0700, Frank van der Linden wrote: > On Wed, Sep 3, 2025 at 9:05 AM Thierry Reding <thierry.red...@gmail.com> > wrote: > > > > On Tue, Sep 02, 2025 at 10:27:01AM -0700, Frank van der Linden wrote: > > > On Tue, Sep 2, 2025 at 8:46 AM Thierry Reding <thierry.red...@gmail.com> > > > wrote: > > > > > > > > From: Thierry Reding <tred...@nvidia.com> > > > > > > > > There is no technical reason why there should be a limited number of CMA > > > > regions, so extract some code into helpers and use them to create extra > > > > functions (cma_create() and cma_free()) that allow creating and freeing, > > > > respectively, CMA regions dynamically at runtime. > > > > > > > > Note that these dynamically created CMA areas are treated specially and > > > > do not contribute to the number of total CMA pages so that this count > > > > still only applies to the fixed number of CMA areas. > > > > > > > > Signed-off-by: Thierry Reding <tred...@nvidia.com> > > > > --- > > > > include/linux/cma.h | 16 ++++++++ > > > > mm/cma.c | 89 ++++++++++++++++++++++++++++++++++----------- > > > > 2 files changed, 83 insertions(+), 22 deletions(-) > > [...] > > > I agree that supporting dynamic CMA areas would be good. However, by > > > doing it like this, these CMA areas are invisible to the rest of the > > > system. E.g. cma_for_each_area() does not know about them. It seems a > > > bit inconsistent that there will now be some areas that are globally > > > known, and some that are not. > > > > That was kind of the point of this experiment. When I started on this I > > ran into the case where I was running out of predefined CMA areas and as > > I went looking for ways on how to fix this, I realized that there's not > > much reason to keep a global list of these areas. And even less reason > > to limit the number of CMA areas to this predefined list. Very little > > code outside of the core CMA code even uses this. > > > > There's one instance of cma_for_each_area() that I don't grok. There's > > another early MMU fixup for CMA areas in 32-bit ARM that. Other than > > that there's a few places where the total CMA page count is shown for > > informational purposes and I don't know how useful that really is > > because totalcma_pages doesn't really track how many pages are used for > > CMA, but pages that could potentially be used for CMA. > > > > And that's about it. > > > > It seems like there are cases where we might really need to globally > > know about some of these areas, specifically ones that are allocated > > very early during boot and then used for very specific purposes. > > > > However, it seems to me like CMA is more universally useful than just > > for these cases and I don't see the usefulness of tracking these more > > generic uses. > > > > > I am being somewhat selfish here, as I have some WIP code that needs > > > the global list :-) But I think the inconsistency is a more general > > > point than just what I want (and the s390 code does use > > > cma_for_each_area()). Maybe you could keep maintaining a global > > > structure containing all areas? > > > > If it's really useful to be able to access all CMA areas, then we could > > easily just add them all to a global linked list upon activation (we may > > still want/need to keep the predefined list around for all those early > > allocation cases). That way we'd get the best of both worlds. > > > > > What do you think are the chances of running out of the global count > > > of areas? > > > > Well, I did run out of CMA areas during the early VPR testing because I > > was initially testing with 16 areas and a different allocation scheme > > that turned out to cause too many resizes in common cases. > > > > However, given that the default is 8 on normal systems (20 on NUMA) and > > is configurable, it means that even with restricting this to 4 for VPR > > doesn't always guarantee that all 4 are available. Again, yes, we could > > keep bumping that number, but why not turn this into something a bit > > more robust where nobody has to know or care about how many there are? > > > > > Also, you say that "these are treated specially and do not contribute > > > to the number of total CMA pages". But, if I'm reading this right, you > > > do call cma_activate_area(), which will do > > > init_cma_reserved_pageblock() for each pageblock in it. Which adjusts > > > the CMA counters for the zone they are in. But your change does not > > > adjust totalcma_pages for dynamically created areas. That seems > > > inconsistent, too. > > > > I was referring to just totalcma_pages that isn't impacted by these > > dynamically allocated regions. This is, again, because I don't see why > > that information would be useful. It's a fairly easy change to update > > that value, so if people prefer that, I can add that. > > > > I don't see an immediate connection between totalcma_pages and > > init_cma_reserved_pageblock(). I thought the latter was primarily useful > > for making sure that the CMA pages can be migrated, which is still > > critical for this use-case. > > My comment was about statistics, they would be inconsistent after your > change. E.g. currently, totalcma_pages is equal to the sum of CMA > pages in each zone. But that would no longer be true, and applications > / administrators looking at those statistics might see the > inconsistency (between meminfo and vmstat) and wonder what's going on. > It seems best to keep those numbers in sync. > > In general, I think it's fine to support dynamic allocation, and I > agree with your arguments that it doesn't seem right to set the number > of CMA areas via a config option. I would just like there to be a > canonical way to find all CMA areas.
Okay, so judging by your and David's feedback, it sounds like I should add a bit of code to track dynamically allocated areas within a global list, along with the existing predefined regions and keep totalcma_pages updated so that the global view is consistent. I'll look into that. Thanks for the feedback. Thierry
signature.asc
Description: PGP signature
