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

[Kirill Tkhai]

On 31.08.2021 17:16, Nikita Yushchenko wrote:
>>> +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?

MSECS_PER_DAY looks good.
 
>>
>>> +            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.