Description of problem: If I'm looking at most xlators, they use it with something like (random example): gf_mt_jbr_end which is defined in xlators/experimental/jbr-server/src/jbr-internal.h as: enum { gf_mt_jbr_private_t = gf_common_mt_end + 1, gf_mt_jbr_fd_ctx_t, gf_mt_jbr_inode_ctx_t, gf_mt_jbr_dirty_t, gf_mt_jbr_end }; What is the value of gf_common_mt_end ? Some number, defined in libglusterfs/src/mem-types.h as 150 or so (did not really count or looked, but seen it's quite a large list) So gf_mt_jbr_end ends up being 155 or so. Then in xlator_mem_acct_init(), I see these: xl->mem_acct = MALLOC(sizeof(struct mem_acct) + sizeof(struct mem_acct_rec) * num_types); Which seems to me that we are allocating plenty of mem_acct_rec structs, even if we only need 5 or so? We are clearly allocating and then memsetting and probably not using way too many mem_acct_rec records. Version-Release number of selected component (if applicable): How reproducible: Steps to Reproduce: 1. 2. 3. Actual results: Expected results: Additional info:
(In reply to Yaniv Kaul from comment #0) > Description of problem: > If I'm looking at most xlators, they use it with something like (random > example): > gf_mt_jbr_end > > which is defined in xlators/experimental/jbr-server/src/jbr-internal.h as: > enum { > gf_mt_jbr_private_t = gf_common_mt_end + 1, > gf_mt_jbr_fd_ctx_t, > gf_mt_jbr_inode_ctx_t, > gf_mt_jbr_dirty_t, > gf_mt_jbr_end > }; > > > What is the value of gf_common_mt_end ? Some number, defined in > libglusterfs/src/mem-types.h as 150 or so (did not really count or looked, > but seen it's quite a large list) > > So gf_mt_jbr_end ends up being 155 or so. > Then in xlator_mem_acct_init(), I see these: > xl->mem_acct = MALLOC(sizeof(struct mem_acct) + > sizeof(struct mem_acct_rec) * num_types); > > Which seems to me that we are allocating plenty of mem_acct_rec structs, > even if we only need 5 or so? We are clearly allocating and then memsetting > and probably not using way too many mem_acct_rec records. > All memory accounting happens per xlator. When a xlator invokes a libglusterfs function, any memory allocation happening there leverages a common memory type and the accounting happens in xl->mem_acct[common_mt]. Hence it is not very easy to determine which memory type record would not be used and so the allocation in init() looks ok to me.
(In reply to Vijay Bellur from comment #1) > (In reply to Yaniv Kaul from comment #0) > > Description of problem: > > If I'm looking at most xlators, they use it with something like (random > > example): > > gf_mt_jbr_end > > > > which is defined in xlators/experimental/jbr-server/src/jbr-internal.h as: > > enum { > > gf_mt_jbr_private_t = gf_common_mt_end + 1, > > gf_mt_jbr_fd_ctx_t, > > gf_mt_jbr_inode_ctx_t, > > gf_mt_jbr_dirty_t, > > gf_mt_jbr_end > > }; > > > > > > What is the value of gf_common_mt_end ? Some number, defined in > > libglusterfs/src/mem-types.h as 150 or so (did not really count or looked, > > but seen it's quite a large list) > > > > So gf_mt_jbr_end ends up being 155 or so. > > Then in xlator_mem_acct_init(), I see these: > > xl->mem_acct = MALLOC(sizeof(struct mem_acct) + > > sizeof(struct mem_acct_rec) * num_types); > > > > Which seems to me that we are allocating plenty of mem_acct_rec structs, > > even if we only need 5 or so? We are clearly allocating and then memsetting > > and probably not using way too many mem_acct_rec records. > > > > All memory accounting happens per xlator. When a xlator invokes a > libglusterfs function, any memory allocation happening there leverages a > common memory type and the accounting happens in xl->mem_acct[common_mt]. > Hence it is not very easy to determine which memory type record would not be > used and so the allocation in init() looks ok to me. I think the real issue comes from the fact that common is not really common. If we had real 10 or 100 common types, which was actually shared by many xlators, then fine, I can understand that. But the reality is that there is an abuse of the common types. Completely random examples: gf_common_mt_tw_ctx - used exactly once, in ctx.c gf_mt_sql_cbk_args_t - not used AT ALL? gf_common_mt_rdma_arena_mr - used exactly once, in rdma.c So I think the real issue is the abuse. If we had a real concise list, then the overuse wouldn't have been a big deal (of course, we are talking here on few KBs or 10's of KBs, so it's not such a big deal anyway, but it just looks wrong to me)
(In reply to Yaniv Kaul from comment #2) > (In reply to Vijay Bellur from comment #1) > > (In reply to Yaniv Kaul from comment #0) > > > Description of problem: > > > If I'm looking at most xlators, they use it with something like (random > > > example): > > > gf_mt_jbr_end > > > > > > which is defined in xlators/experimental/jbr-server/src/jbr-internal.h as: > > > enum { > > > gf_mt_jbr_private_t = gf_common_mt_end + 1, > > > gf_mt_jbr_fd_ctx_t, > > > gf_mt_jbr_inode_ctx_t, > > > gf_mt_jbr_dirty_t, > > > gf_mt_jbr_end > > > }; > > > > > > > > > What is the value of gf_common_mt_end ? Some number, defined in > > > libglusterfs/src/mem-types.h as 150 or so (did not really count or looked, > > > but seen it's quite a large list) > > > > > > So gf_mt_jbr_end ends up being 155 or so. > > > Then in xlator_mem_acct_init(), I see these: > > > xl->mem_acct = MALLOC(sizeof(struct mem_acct) + > > > sizeof(struct mem_acct_rec) * num_types); > > > > > > Which seems to me that we are allocating plenty of mem_acct_rec structs, > > > even if we only need 5 or so? We are clearly allocating and then memsetting > > > and probably not using way too many mem_acct_rec records. > > > > > > > All memory accounting happens per xlator. When a xlator invokes a > > libglusterfs function, any memory allocation happening there leverages a > > common memory type and the accounting happens in xl->mem_acct[common_mt]. > > Hence it is not very easy to determine which memory type record would not be > > used and so the allocation in init() looks ok to me. > > I think the real issue comes from the fact that common is not really common. > If we had real 10 or 100 common types, which was actually shared by many > xlators, then fine, I can understand that. But the reality is that there is > an abuse of the common types. > Completely random examples: > gf_common_mt_tw_ctx - used exactly once, in ctx.c > gf_mt_sql_cbk_args_t - not used AT ALL? > gf_common_mt_rdma_arena_mr - used exactly once, in rdma.c > > So I think the real issue is the abuse. If we had a real concise list, then > the overuse wouldn't have been a big deal (of course, we are talking here on > few KBs or 10's of KBs, so it's not such a big deal anyway, but it just > looks wrong to me) Yeah, agree on that. We could clean up the ones that are not used or seldom used. I am also thinking about a second table for sparingly used memory types. Will update when I have a patch here.
https://review.gluster.org/#/c/glusterfs/+/21657/ removed some unused ones and marked those that are used only once and can be moved - this will have to be done in a follow-up patch.
This bug is moved to https://github.com/gluster/glusterfs/issues/904, and will be tracked there from now on. Visit GitHub issues URL for further details