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

[David Hildenbrand <david@redhat.com>]

>>
> 
> 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