On Fri 2016-06-10 15:29:05, Andrew Morton wrote: > On Thu, 9 Jun 2016 11:07:10 -0400 Steven Rostedt <rostedt@xxxxxxxxxxx> wrote: > > > On Thu, 9 Jun 2016 15:51:56 +0200 > > Petr Mladek <pmladek@xxxxxxxx> wrote: > > > > > A good practice is to prefix the names of functions and macros > > > by the name of the subsystem. > > > > > > The kthread worker API is a mix of classic kthreads and workqueues. > > > Each worker has a dedicated kthread. It runs a generic function > > > that process queued works. It is implemented as part of > > > the kthread subsystem. > > > > > > This patch renames the existing kthread worker API to use > > > the corresponding name from the workqueues API prefixed by > > > kthread_/KTHREAD_: > > > > > > DEFINE_KTHREAD_WORKER() -> KTHREAD_DECLARE_WORKER() > > > DEFINE_KTHREAD_WORK() -> KTHREAD_DECLARE_WORK() > > > DEFINE_KTHREAD_WORKER_ONSTACK() -> KTHREAD_DECLARE_WORKER_ONSTACK() > > > DEFINE_KTHREAD_WORKER_ONSTACK() -> KTHREAD_DECLARE_WORKER_ONSTACK() > > > __init_kthread_worker() -> __kthread_init_worker() > > > init_kthread_worker() -> kthread_init_worker() > > > init_kthread_work() -> kthread_init_work() > > > insert_kthread_work() -> kthread_insert_work() > > > queue_kthread_work() -> kthread_queue_work() > > > flush_kthread_work() -> kthread_flush_work() > > > flush_kthread_worker() -> kthread_flush_worker() > > > > > > > I know that Andrew suggested this, but I didn't get a chance to respond > > to his email due to traveling. > > > > Does this mean we are going to change all APIs like this? Because we > > pretty much use this type of naming everywhere. That is, we start with > > "DEFINE_*" and "DECLARE_*" commonly. As well as "init_*". > > > > For example DEFINE_PER_CPU(), DEFINE_SPINLOCK(), DEFINE_LGLOCK(), > > DEFINE_MUTEX(), DEFINE_RES_MEME(), DEFINE_TIMER(), DEFINE_IDA(), > > DEFINE_NFS4_*(), and the list goes on. Just do a grep in > > include/linux/*.h for DEFINE_ and DECLARE_. > > Yes, there's so much precedence that DEFINE_KTHREAD_WORKER() and > friends can/should be left as-is. > > But I do think that init_kthread_worker() is a sore thumb and should > become kthread_worker_init() (not kthread_worker_init()) OK, all wants to keep DEFINE stuff as is: DEFINE_KTHREAD_WORKER() stay DEFINE_KTHREAD_WORK() stay DEFINE_KTHREAD_WORKER_ONSTACK() stay DEFINE_KTHREAD_WORKER_ONSTACK() stay Nobody was against renaming the non-init functions: insert_kthread_work() -> kthread_insert_work() queue_kthread_work() -> kthread_queue_work() flush_kthread_work() -> kthread_flush_work() flush_kthread_worker() -> kthread_flush_worker() Now, the question seem to be the init() functions. Andrew would prefer: __init_kthread_worker() -> __kthread_worker_init() init_kthread_worker() -> kthread_worker_init() init_kthread_work() -> kthread_work_init() AFAIK, Steven would prefer to keep it __init_kthread_worker() stay as is init_kthread_worker() stay as is init_kthread_work() stay as is I would personally prefer the way from this patch: __init_kthread_worker() -> __kthread_init_worker() init_kthread_worker() -> kthread_init_worker() init_kthread_work() -> kthread_init_work() I have several reasons: 1. The init functions will be used close to the other functions in the code. It will be easier if all functions use the same naming scheme. Here are some snippets: kthread_init_work(&w_data->balancing_work, clamp_balancing_func); kthread_init_delayed_work(&w_data->idle_injection_work, clamp_idle_injection_func); kthread_queue_work(w_data->worker, &w_data->balancing_work); or kthread_init_delayed_work(&kmemleak_scan_work, kmemleak_scan_func); kmemleak_scan_worker = kthread_create_worker(0, "kmemleak"); 2. We are going to add kthread_destroy_worker() which would need to be another exception. Also this function will be used together with the others, for example: kthread_cancel_delayed_work_sync(&rb_producer_hammer_work); kthread_destroy_worker(rb_producer_worker); Also here the same naming scheme will help. 3. It is closer to the workqueues API, so it reduces confusion. 4. Note that there are already several precedents, for example: amd_iommu_init_device() free_area_init_node() jump_label_init_type() regmap_init_mmio_clk() Andrew, Steven, are you really so strongly against my version of the init functions, please? > > Also, are you sure that we should change the DEFINE to a DECLARE, > > because DEFINE is used to create the object in question, DECLARE is for > > header files: > > Yes2, these macros expand to definitions, not to declarations. Shame on me. I played with many variants, looked for the most consistent solution, and got lost in all the constrains. Best Regards, Petr -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
