Re: [PATCH v2 1/4] mm/ksm: add ksm advisor

[David Hildenbrand]

> >
>
> The min cpu case is to make sure that we scan fast enough to be able to
> react fast enough to the changes in the number of pages. This helps in
> determining in how quick we want to react to changes. This helps
> especially with the startup phase of applications.
>
> We can certainly only set a default value, that is not exposed in sysfs.

Less toggles is better. So if we can just use some sane starting 
default, that would be great.

> > > +/**
> > > + * struct advisor_ctx - metadata for KSM advisor
> > > + * @start_scan: start time of the current scan
> > > + * @scan_time: scan time of previous scan
> > > + * @change: change in percent to pages_to_scan parameter
> > > + * @cpu_percent: average cpu percent usage of the ksmd thread for the last scan
> > > + */
> > > +struct advisor_ctx {
> > > +	ktime_t start_scan;
> > > +	unsigned long scan_time;
> > > +	unsigned long change;
> > > +	unsigned long long cpu_time;
> > > +};
> > > +static struct advisor_ctx advisor_ctx;
> > > +
> > > +/* Define different advisor's */
> > > +enum ksm_advisor_type {
> > > +	KSM_ADVISOR_NONE,
> > > +	KSM_ADVISOR_FIRST = KSM_ADVISOR_NONE,
> >
> > Unused, better drop it. 0 is the implicit first one.
> Will change it accordingly.
>
> > > +	KSM_ADVISOR_SCAN_TIME,
> > > +	KSM_ADVISOR_LAST = KSM_ADVISOR_SCAN_TIME
> >
> > Instead of "_LAST", maybe use "_COUNT" and use that when checking for valid
> > values.
> >
> > But: we likely want to store "strings" instead of magic numbers from user space
> > instead.
>
> Any recommendation for the naming of the parameters when I switch to
> strings?

Probably just "none" and "scan-time" ?

> > > +}
> > > +
> > > +static void run_advisor(void)
> > > +{
> > > +	if (ksm_advisor == KSM_ADVISOR_SCAN_TIME) {
> > > +		s64 scan_time;
> > > +
> > > +		/* Convert scan time to seconds */
> > > +		scan_time = ktime_ms_delta(ktime_get(), advisor_ctx.start_scan);
> > > +		scan_time = div_s64(scan_time, MSEC_PER_SEC);
> > > +		scan_time = scan_time ? scan_time : 1;
> > > +
> > > +		scan_time_advisor((unsigned long)scan_time);
> > > +	}
> >
> > We could have rescheduled in the meantime, right? Doesn't that mean that our CPU
> > load consumption might be wrong in some cases?
> Does it matter? I'm interested how long it takes to complete the scan,
> including any scheduling.

But isn't this also required to compute CPU load, so you can stay 
between min-load and max-load?

- ksm_advisor_min_cpu (minimum value for cpu percent usage)
- ksm_advisor_max_cpu (maximum value for cpu percent usage)

Likely, you want to exclude any rescheduling from there?

I'll have to recheck the logic.

--
Cheers,

David / dhildenb