[LU-9010] use static initialization for statically allocated objects when possible Created: 11/Jan/17  Updated: 05/Jul/19  Resolved: 27/Jun/19

Status: Resolved
Project: Lustre
Component/s: None
Affects Version/s: None
Fix Version/s: Lustre 2.13.0

Type: Improvement Priority: Minor
Reporter: John Hammond Assignee: Arshad Hussain
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Related
is related to LU-6142 Enforce Linux kernel coding style in ... Open
Rank (Obsolete): 9223372036854775807

 Description   

Statically allocated completions, list heads, mutexes, timer lists, atomics, spinlocks, wait queues, etc should be statically initialized using kernel provided macros. Dynamic initialization can be identified by auditing module_init() functions:

static __init int ptlrpc_init(void)
{
        ...

#if RS_DEBUG
        spin_lock_init(&ptlrpc_rs_debug_lock);
#endif
        INIT_LIST_HEAD(&ptlrpc_all_services);
        mutex_init(&ptlrpc_all_services_mutex);
        mutex_init(&pinger_mutex);
        mutex_init(&ptlrpcd_mutex);
        ...
}

Here spin_lock_init(&ptlrpc_rs_debug_lock) can be dropped if prlrpc_rs_debug_lock is defined using DEFINE_SPINLOCK() and similarly the other initializations shown above.

Regex searches can also identify top level definitions of these structures that fail to use the initializer macros:

lustre-release$ git grep -E -e '^(static[ \t]+)?struct[ \t]+((completion)|(list_head)|(mutex)|(timer_list))' -e '^(static[ \t]+)((atomic_t)|(spinlock_t)|(wait_queue_head_t))'
libcfs/include/libcfs/util/list.h:struct list_head {
libcfs/libcfs/debug.c:static wait_queue_head_t debug_ctlwq;
libcfs/libcfs/tracefile.c:static atomic_t cfs_tage_allocated = ATOMIC_INIT(0);
libcfs/libcfs/watchdog.c:static struct completion lcw_start_completion;
libcfs/libcfs/watchdog.c:static struct completion  lcw_stop_completion;
libcfs/libcfs/watchdog.c:static wait_queue_head_t lcw_event_waitq;
libcfs/libcfs/watchdog.c:static struct list_head lcw_pending_timers = LIST_HEAD_INIT(lcw_pending_timers);
lnet/include/lnet/lib-lnet.h:struct list_head *lnet_mt_match_head(struct lnet_match_table *mtable,
lnet/include/lnet/nidstr.h:struct list_head;
lnet/klnds/o2iblnd/o2iblnd.c:struct list_head *
lnet/klnds/o2iblnd/o2iblnd.h:struct list_head *kiblnd_pool_alloc_node(kib_poolset_t *ps);
lnet/lnet/lib-ptl.c:struct list_head *
lnet/lnet/module.c:static struct mutex lnet_config_mutex;
lustre/ldlm/ldlm_internal.h:struct list_head *ldlm_namespace_inactive_list(enum ldlm_side client)
lustre/ldlm/ldlm_lockd.c:static struct mutex    ldlm_ref_mutex;
lustre/ldlm/ldlm_lockd.c:static spinlock_t waiting_locks_spinlock;   /* BH lock (timer) */
lustre/ldlm/ldlm_lockd.c:static struct list_head waiting_locks_list;
lustre/ldlm/ldlm_lockd.c:static struct timer_list waiting_locks_timer;
lustre/ldlm/ldlm_pool.c:static struct completion ldlm_pools_comp;
lustre/ldlm/ldlm_reclaim.c:static atomic_t                      ldlm_nr_reclaimer;
lustre/ldlm/ldlm_resource.c:struct mutex ldlm_srv_namespace_lock;
lustre/ldlm/ldlm_resource.c:struct list_head ldlm_srv_namespace_list;
lustre/ldlm/ldlm_resource.c:struct mutex ldlm_cli_namespace_lock;
lustre/ldlm/ldlm_resource.c:struct list_head ldlm_cli_active_namespace_list;
lustre/ldlm/ldlm_resource.c:struct list_head ldlm_cli_inactive_namespace_list;
lustre/lfsck/lfsck_lib.c:static struct list_head lfsck_instance_list;
lustre/lfsck/lfsck_lib.c:static struct list_head lfsck_ost_orphan_list;
lustre/lfsck/lfsck_lib.c:static struct list_head lfsck_mdt_orphan_list;
lustre/mgc/mgc_request.c:static struct list_head config_llog_list = LIST_HEAD_INIT(config_llog_list);
lustre/mgc/mgc_request.c:static wait_queue_head_t      rq_waitq;
lustre/mgc/mgc_request.c:static atomic_t mgc_count = ATOMIC_INIT(0);
lustre/obdclass/cl_object.c:static spinlock_t *cl_object_attr_guard(struct cl_object *o)
lustre/obdclass/class_obd.c:struct list_head obd_types;
lustre/obdclass/genops.c:static struct list_head obd_zombie_imports;
lustre/obdclass/genops.c:static struct list_head obd_zombie_exports;
lustre/obdclass/genops.c:static spinlock_t  obd_zombie_impexp_lock;
lustre/obdclass/genops.c:struct list_head obd_stale_exports;
lustre/obdclass/genops.c:static struct completion       obd_zombie_start;
lustre/obdclass/genops.c:static struct completion       obd_zombie_stop;
lustre/obdclass/genops.c:static wait_queue_head_t       obd_zombie_waitq;
lustre/obdclass/kernelcomm.c:static struct list_head kkuc_groups[KUC_GRP_MAX+1] = {};
lustre/obdclass/local_storage.c:static struct list_head ls_list_head = LIST_HEAD_INIT(ls_list_head);
lustre/obdclass/lu_object.c:static struct list_head lu_device_types;
lustre/obdclass/lu_object.c:static struct list_head lu_sites;
lustre/obdclass/lu_object.c:static atomic_t lu_key_initing_cnt = ATOMIC_INIT(0);
lustre/obdclass/lu_object.c:static struct list_head lu_context_remembered;
lustre/obdclass/lu_ref.c:static struct list_head lu_ref_refs;
lustre/obdclass/lu_ref.c:static spinlock_t lu_ref_refs_guard;
lustre/obdclass/lustre_handles.c:static spinlock_t handle_base_lock;
lustre/obdclass/lustre_peer.c:static struct list_head   g_uuid_list;
lustre/obdclass/lustre_peer.c:static spinlock_t g_uuid_lock;
lustre/obdclass/obd_config.c:static struct list_head lustre_profile_list =
lustre/obdclass/obd_mount_server.c:static struct list_head server_mount_info_list =
lustre/obdclass/obd_mount_server.c:static struct list_head lwp_register_list =
lustre/osc/osc_request.c:struct list_head osc_shrink_list = LIST_HEAD_INIT(osc_shrink_list);
lustre/osd-ldiskfs/osd_iam.c:static struct list_head iam_formats = LIST_HEAD_INIT(iam_formats);
lustre/osd-ldiskfs/osd_oi.c:static struct mutex oi_init_lock;
lustre/osp/osp_sync.c:static struct list_head osp_id_tracker_list =
lustre/ptlrpc/client.c:static spinlock_t ptlrpc_last_xid_lock;
lustre/ptlrpc/gss/gss_krb5_mech.c:static spinlock_t krb5_seq_lock;
lustre/ptlrpc/gss/gss_mech_switch.c:static struct list_head registered_mechs = LIST_HEAD_INIT(registered_mechs);
lustre/ptlrpc/gss/gss_pipefs.c:static atomic_t upcall_seq = ATOMIC_INIT(0);
lustre/ptlrpc/gss/gss_pipefs.c:static struct list_head upcall_lists[MECH_MAX];
lustre/ptlrpc/gss/gss_pipefs.c:static spinlock_t upcall_locks[MECH_MAX];
lustre/ptlrpc/gss/gss_svc_upcall.c:static spinlock_t __ctx_index_lock;
lustre/ptlrpc/pack_generic.c:struct list_head ptlrpc_rs_debug_lru =
lustre/ptlrpc/pinger.c:struct mutex pinger_mutex;
lustre/ptlrpc/pinger.c:static struct list_head pinger_imports =
lustre/ptlrpc/pinger.c:static struct list_head timeout_list =
lustre/ptlrpc/pinger.c:static wait_queue_head_t pet_waitq;
lustre/ptlrpc/pinger.c:static struct list_head   pet_list;
lustre/ptlrpc/ptlrpcd.c:struct mutex ptlrpcd_mutex;
lustre/ptlrpc/sec.c:static atomic_t sptlrpc_sec_id = ATOMIC_INIT(1);
lustre/ptlrpc/sec_config.c:static struct mutex sptlrpc_conf_lock;
lustre/ptlrpc/sec_config.c:static struct list_head sptlrpc_confs;
lustre/ptlrpc/sec_gc.c:static struct mutex sec_gc_mutex;
lustre/ptlrpc/sec_gc.c:static spinlock_t sec_gc_list_lock;
lustre/ptlrpc/sec_gc.c:static struct list_head sec_gc_list;
lustre/ptlrpc/sec_gc.c:static spinlock_t sec_gc_ctx_list_lock;
lustre/ptlrpc/sec_gc.c:static struct list_head sec_gc_ctx_list;
lustre/ptlrpc/sec_gc.c:static atomic_t sec_gc_wait_del = ATOMIC_INIT(0);
lustre/ptlrpc/service.c:struct list_head ptlrpc_all_services;
lustre/ptlrpc/service.c:struct mutex ptlrpc_all_services_mutex;
lustre/quota/qsd_config.c:static struct list_head qfs_list = LIST_HEAD_INIT(qfs_list);
lustre/tests/it_test.c:static struct list_head header = LIST_HEAD_INIT(header);

Note that there are some easily identified false positives among the matches.

Often this means that the structure can be declared static in the file where it is defined.

One patch per module (where needed) would probably be about right. Except for LNet which is by far the worst offender. Fixing it would mean breaking up the the_lnet object. So it wouldn't be a small change but the change could easily be broken into small chunks by working file by file. For example several members (including ln_eq_waitq) are only used in lib-eq.c and could be removed and replaced with a static object in lib-eq.c (static DECLARE_WAIT_QUEUE_HEAD(lnet_eq_waitq)).

I'll upload a patch for ldlm as an example.



 Comments   
Comment by Gerrit Updater [ 11/Jan/17 ]

John L. Hammond (john.hammond@intel.com) uploaded a new patch: https://review.whamcloud.com/24824
Subject: LU-9010 ldlm: use static initializer macros where possible
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 37dcc8dfe4484fcd81ddf510d8a3d1ac043395b5

Comment by Jinshan Xiong (Inactive) [ 11/Jan/17 ]

Is there any other benefits for us to do so besides one line code less in the implementation?

Comment by Gerrit Updater [ 11/Jan/17 ]

John L. Hammond (john.hammond@intel.com) uploaded a new patch: https://review.whamcloud.com/24827
Subject: LU-9010 obdclass: use static initializer macros where possible
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: fe719739e1d01f09eb378050fd51071ef45b83ba

Comment by John Hammond [ 11/Jan/17 ]

> Is there any other benefits for us to do so besides one line code less in the implementation?

Safer code.

Comment by James A Simmons [ 11/Jan/17 ]

Also upstream likes this kind of work alot

Comment by John Hammond [ 11/Jan/17 ]

Steve, can you add this to your pipeline?

Comment by Steve Guminski (Inactive) [ 11/Jan/17 ]

Sure, I'll add this to my queue.

Comment by Gerrit Updater [ 23/Mar/17 ]

Oleg Drokin (oleg.drokin@intel.com) merged in patch https://review.whamcloud.com/24827/
Subject: LU-9010 obdclass: use static initializer macros where possible
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: d675a1fe48be5d86a3fdb9ccc9542a748cc007a0

Comment by Gerrit Updater [ 03/Jun/17 ]

Oleg Drokin (oleg.drokin@intel.com) merged in patch https://review.whamcloud.com/24824/
Subject: LU-9010 ldlm: use static initializer macros where possible
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: cdfb271f37288c038f5b525fe7764df59d92e0b3

Comment by James A Simmons [ 03/Jun/17 ]

I assume more work is left

Comment by Gerrit Updater [ 28/Dec/18 ]

Arshad Hussain (arshad.super@gmail.com) uploaded a new patch: https://review.whamcloud.com/33932
Subject: LU-9010 lnet: Change static defines to use macro for module.c
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 43b1cd8ff3b5e66856b5ccbac7d5e8d1015e0de6

Comment by Gerrit Updater [ 28/Dec/18 ]

Arshad Hussain (arshad.super@gmail.com) uploaded a new patch: https://review.whamcloud.com/33933
Subject: LU-9010 libcfs: Change static defines to use macro for watchdog.c
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 74617c39ad87a17c09724b7f166ebbb4ea395e14

Comment by Gerrit Updater [ 28/Dec/18 ]

Arshad Hussain (arshad.super@gmail.com) uploaded a new patch: https://review.whamcloud.com/33934
Subject: LU-9010 ptlrpc: Change static defines to use macro for gss_svc_upcall.c
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: d78c59dd8603318d3ea64933426dc690a621a254

Comment by Gerrit Updater [ 28/Dec/18 ]

Arshad Hussain (arshad.super@gmail.com) uploaded a new patch: https://review.whamcloud.com/33935
Subject: LU-9010 tests: Change static defines to use macro for kinode.c
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: d6be3ea3d2760d1cbbd732706b69b400c9d47a7c

Comment by Gerrit Updater [ 28/Dec/18 ]

Arshad Hussain (arshad.super@gmail.com) uploaded a new patch: https://review.whamcloud.com/33936
Subject: LU-9010 ptlrpc: Change static defines to use macro for gss_krb5_mech.c
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 07d7655a47a397157022ed0ae96bbdc282cc11df

Comment by Gerrit Updater [ 28/Dec/18 ]

Arshad Hussain (arshad.super@gmail.com) uploaded a new patch: https://review.whamcloud.com/33937
Subject: LU-9010 ptlrpc: Change static defines to use macro for sec_gc.c
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 273afce8f524993bf06e5b3e58ba98959c233f4e

Comment by Gerrit Updater [ 10/Jan/19 ]

Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/33934/
Subject: LU-9010 ptlrpc: Change static defines to use macro for gss_svc_upcall.c
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 8c9a8a0d451cbc7c76e96e8e501f98b169726f53

Comment by Gerrit Updater [ 10/Jan/19 ]

Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/33935/
Subject: LU-9010 tests: Change static defines to use macro for kinode.c
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 92bc4cfabc4e1411a423da0efb23011f3b7c4849

Comment by Gerrit Updater [ 21/Mar/19 ]

Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/33937/
Subject: LU-9010 ptlrpc: Change static defines to use macro for sec_gc.c
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 50c01e02506fbeb50bc36a2ccfcb7037bb65cd2f

Comment by Gerrit Updater [ 04/May/19 ]

Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/33936/
Subject: LU-9010 ptlrpc: Change static defines to use macro for gss_krb5_mech.c
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: ad51768ddb043c5fd2a3c6b8663c66b71dc3f391

Comment by Gerrit Updater [ 25/Jun/19 ]

Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/33932/
Subject: LU-9010 lnet: Change static defines to use macro for module.c
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: bb967468875f967c9463afb19c3fbd27cfdf0688

Comment by Arshad Hussain [ 25/Jun/19 ]

@James Simmons,
Any work more left on this.? This could be closed now.

Comment by James A Simmons [ 27/Jun/19 ]

I think its safe to close this. Might be some we missed but that is what LU-6142 is for

Comment by James A Simmons [ 27/Jun/19 ]

Arshad finished this work off  Great job

Comment by Arshad Hussain [ 05/Jul/19 ]

Thank you James.

... and it looks like LU-6142 is also going to get resolved soon.

Comment by James A Simmons [ 05/Jul/19 ]

I think we have more to go. Their are things Lustre checkpatch doesn't report that the upstream linux kernel one does. Also I need to push a patch to tell people to stop with the __u32 type usage in kernel code. The tabs is nearly done. We also have LNet which is a complete mess !!!

Generated at Sat Feb 10 02:22:29 UTC 2024 using Jira 9.4.14#940014-sha1:734e6822bbf0d45eff9af51f82432957f73aa32c.