On Tue, Jun 09, 2026 at 05:44:31PM -0400, Gregory Price wrote: > On Tue, Jun 09, 2026 at 04:08:08PM -0400, Michael S. Tsirkin wrote: > > On Tue, Jun 09, 2026 at 01:44:37PM -0400, Gregory Price wrote: > > > On Tue, Jun 09, 2026 at 11:53:20AM -0400, Michael S. Tsirkin wrote: > > > > --- a/mm/page_reporting.c > > > > +++ b/mm/page_reporting.c > > > > @@ -174,10 +174,10 @@ page_reporting_cycle(struct > > > > page_reporting_dev_info *prdev, struct zone *zone, > > > > * list processed. This should result in us reporting all pages > > > > on > > > > * an idle system in about 30 seconds. > > > > * > > > > - * The division here should be cheap since > > > > PAGE_REPORTING_CAPACITY > > > > - * should always be a power of 2. > > > > + * The division here uses integer division; capacity need > > > > + * not be a power of 2. > > > > */ > > > > - budget = DIV_ROUND_UP(area->nr_free, PAGE_REPORTING_CAPACITY * > > > > 16); > > > > + budget = DIV_ROUND_UP(area->nr_free, prdev->capacity * 16); > > > > > > > > > > Initial look - is there a div-by-0 here? I noticed the old check > > > prevents this from being (0 * 16), but i don't see (on first pass) > > > the same check anywhere. > > > > > > Unless this line below always forces the above to be a > > > PAGE_REPORTING_CAPCAITY if it's set to 0. > > > > It does, does it not? > > > > > > + if (!prdev->capacity || prdev->capacity > > > > > PAGE_REPORTING_CAPACITY) > > > > + prdev->capacity = PAGE_REPORTING_CAPACITY; > > > > + > > > > > > It's worth making this corner condition a little more obvious. > > > > > > The code intends for > > > > > > if (capacity == 0) > > > capacity = PAGE_REPORTING_CAPACITY > > > > > > but that's not reflected in the changelog as a default value. > > > > > > When happens if a driver sets (capacity=0) either on purpose (???) > > > > what would the purpose be? if you don't want reporting do not register. > > > > > or > > > because there's a bug (???) > > > > exactly ??? since where are we practicing defensive programming in kernel > > APIs? > > > > > and then page_reporting.c forces it up to > > > 32? > > > > > > There's something to improve here. > > > > > > ~Gregory > > > > > > So I'll update the commit log to mention PAGE_REPORTING_CAPACITY. > > And maybe a comment near capacity field? > > Should be enough? > > I suppose the question is whether capacity=0 should cause a WARN (i.e. > only a bug explains that value), or if capacity=0 means something > special (i.e. use the default) and therefore that should be documented. > I don't know which of these is the case, but if it's the latter than > that deserves a comment yes. > > ~Gregory
It's the default. Will document.
