+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.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
