[Devel] [PATCH] mm: high order allocation detector

[Nikita Yushchenko]

>> +static void hoad_reset_counter(struct work_struct *work)
>> +{
>> +	struct hoad_order_info *hoi = container_of(work,
>> +			struct hoad_order_info, reset_counter_work.work);
>> +
>> +	atomic_set(&hoi->counter, 0);
> 
> Do we really need work to do such simple action? Why plain timer is not enough?

Ok, will convert to timer.

>> +static void hoad_send_uevent(struct work_struct *work)
>> +{
>> +	char order_string[16];
>> +	char *envp[] = { order_string, NULL };
> 
> Please, use new lines after declarations.

Ok

> 
>> +	sprintf(order_string, "ORDER=%d", hoad_uevent_order);
>> +	kobject_uevent_env(hoad_kobj, KOBJ_CHANGE, envp);
>> +}
>> +static DECLARE_WORK(hoad_send_uevent_work, hoad_send_uevent);
>> +
>> +static void hoad_resume(struct work_struct *work)
>> +{
>> +	hoad_uevent_order = 0;
> 
> The same is here: do we really need work here?

Ok, will convert to timer

>> +	if (oldhoi) {
>> +		synchronize_rcu();
>> +		cancel_delayed_work_sync(&oldhoi->reset_counter_work);
> 
> Why do we completely cancel this? We possible updated single order entity,
> don't other orders still need to be reset?

It is per-order, and accesses hoad_order_info container.
Cancelling one for 'oldhoi' before freeing 'oldhoi'.

After converting to timer, this situation will remain.

>> +			p += sprintf(p, "order %u: %u/%u in %u/%u msecs\n",
>> +					order, counter, hoi->max_allocs,
>> +					msecs, jiffies_to_msecs(hoi->interval));
> 
> Any guarantees all printed in this function data fits provided @buf?

Yes, buf is a full page, number of orders is limited (<20), this leaves >200 bytes per order, which is 
more than enough.

>> +		if (msecs > 1000 * 60 * 60 * 24 * 5)
> 
> Every magic number should be described with #define.

Do you want separate defines for every number?
Or things like "5 * MSECS_PER_DAY' can work?

> 
>> +			ret = -EINVAL;
>> +		else {
>> +do_resume:
>> +			d = msecs_to_jiffies(msecs);
>> +			hoad_resume_jiffies = jiffies + d;
>> +			cancel_delayed_work_sync(&hoad_resume_work);
>> +			schedule_delayed_work(&hoad_resume_work, d);
> 
> It looks like we may use mod_delayed_work(system_wq, ...) here instead of two calls above.

This will likely get changed to mod_timer()

>> +	}
>> +	else if (!strcmp(q, "resume")) {
> 
> Please, move this "else if" up to the same line with "}" bracket

Sure - looks like a typo

>> +		msecs = 0;
>> +		goto do_resume;
>> +	} else
>> +		ret = -EINVAL;
> 
> In case of you have "{}" brackets in a single branch of if-else, then every if, else if, else
> branch has to have "{}" brackets even if it's one liner.

Ok

>> +	struct kset *kset = kset = kset_create_and_add("hoad", NULL, mm_kobj);
> 
> 1)kset = kset is here.
> 2)New line is needed after declaration.

Ok

> 
>> +	if (!kset)
>> +		return -ENOMEM;
>> +	hoad_kobj = &kset->kobj;
>> +	hoad_kobj->kset = kset;
>> +
>> +	ret = sysfs_create_file(hoad_kobj, &hoad_control_attr.attr);
>> +	if (ret) {
>> +		hoad_kobj->kset = NULL;
>> +		hoad_kobj = NULL;
>> +		kset_put(kset);
>> +		return ret;
>> +	}
> 
> Why not:
> 
> 	ret = sysfs_create_file(hoad_kobj, &hoad_control_attr.attr);
> 	if (ret) {
> 		kset_put(kset);
> 		return ret;
> 	}
> 
> 	hoad_kobj = &kset->kobj;
> 	hoad_kobj->kset = kset;

I want to:
- use sysfs_create_file(hoad_kobj, ...), not sysfs_create_file(&kset->kobj, ...)
- group configuration together


> Also, as I see in kernel code, the pair bracket for kset_create_and_add() is kset_unregister().
> Why we use kset_put() instead?

Formally, in kset_create_and_add(), sysfs entry gets added and can be opened by other process. So must 
use refcount-based freeing.