Messages in this thread | | | Date | Mon, 9 Sep 2019 19:33:55 +0300 | From | "Kirill A. Shutemov" <> | Subject | Re: [PATCH v9 6/8] mm: Introduce Reported pages |
| |
On Mon, Sep 09, 2019 at 09:25:04AM -0700, Alexander Duyck wrote: > > Proper description for the config option? > > I can add one. However the feature doesn't do anything without a caller > that makes use of it. I guess it would make sense to enable this for > something such as an out-of-tree module to later use.
Description under 'help' section will not make the option user selectable if you leave 'bool' without description.
> > > + mutex_lock(&page_reporting_mutex); > > > + > > > + /* nothing to do if already in use */ > > > + if (rcu_access_pointer(ph_dev_info)) { > > > + err = -EBUSY; > > > + goto err_out; > > > + } > > > > Again, it's from "something went horribly wrong" category. > > Maybe WARN_ON()? > > That one I am not so sure about. Right now we only have one user for the > page reporting interface. My concern is if we ever have more than one we > may experience collisions. The device driver requesting this should > display an error message if it is not able tor register the interface.
Fair enough.
> > > + boundary = kcalloc(MAX_ORDER - PAGE_REPORTING_MIN_ORDER, > > > + sizeof(struct list_head *) * MIGRATE_TYPES, > > > + GFP_KERNEL); > > > > Could you comment here on why this size of array is allocated? > > The calculation is not obvious to a reader. > > Would something like the following work for you? > /* > * Allocate space to store the boundaries for the zone we are > * actively reporting on. We will need to store one boundary > * pointer per migratetype, and then we need to have one of these > * arrays per order for orders greater than or equal to > * PAGE_REPORTING_MIN_ORDER. > */
Ack.
-- Kirill A. Shutemov
| |