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
