Yes, that's much better. I'm chagrined to have not thought of it :( Allen Samuels SanDisk |a Western Digital brand 2880 Junction Avenue, Milpitas, CA 95134 T: +1 408 801 7030| M: +1 408 780 6416 allen.samuels@xxxxxxxxxxx > -----Original Message----- > From: Bartłomiej Święcki [mailto:bartlomiej.swiecki@xxxxxxxxxxxx] > Sent: Thursday, October 20, 2016 2:34 AM > To: Allen Samuels <Allen.Samuels@xxxxxxxxxxx>; Sage Weil > <sweil@xxxxxxxxxx>; ceph-devel@xxxxxxxxxxxxxxx > Subject: Re: mempools everywhere > > Why not just use something like: > > mempool::pool_t& mempool::get_pool(mempool::pool_index_t ix) { > static Mempool table[num_mempools] = { #define P(x) > mempool::pool_t(#x), > DEFINE_MEMORY_POOLS_HELPER(P) > #undef P > }; > return table[ix]; > } > > Constructors of 'table' entries are guaranteed to be executed on the first call > (and to be thread safe in c++0x11) and looks like the extra cost for a check if > 'table' is initialized or not is basically load+cmp+jmp, compilers could use > some clever tricks to make this fast. IIRC this should also ensure that > destructors are called when unloading the module, not 100% sure about that > though. > > Bartek > > > On 10/19/2016 05:09 PM, Allen Samuels wrote: > > Here's a two-line patch to eliminate the memory leak of the mempool > > object itself :) > > > > Mempool& GetPool(index) { > > If (table[index] == nullptr) { > > Static char storage[num_mempools][sizeof(mempool_t)]; > > Table[index] = new ((void *)stroage[index]) mempool_t; > > } > > Return Table[index]; > > } > > > > > > Volia, no leaks :) Makes the .so unload problem easier too :) > > > >> -----Original Message----- > >> From: ceph-devel-owner@xxxxxxxxxxxxxxx [mailto:ceph-devel- > >> owner@xxxxxxxxxxxxxxx] On Behalf Of Allen Samuels > >> Sent: Tuesday, October 18, 2016 10:00 AM > >> To: Sage Weil <sweil@xxxxxxxxxx>; ceph-devel@xxxxxxxxxxxxxxx > >> Subject: RE: mempools everywhere > >> > >>> -----Original Message----- > >>> From: Sage Weil [mailto:sweil@xxxxxxxxxx] > >>> Sent: Tuesday, October 18, 2016 9:52 AM > >>> To: ceph-devel@xxxxxxxxxxxxxxx > >>> Cc: Allen Samuels <Allen.Samuels@xxxxxxxxxxx> > >>> Subject: mempools everywhere > >>> > >>> The mempool PR is more or less ready for final test/merge. Once > >>> it's in place we'd love to use this all over the place so that we > >>> can tell where memory is going. There's one major remaining roadblock, > though: > >>> right now mempool is part of libglobal since it has global state > >>> (the mempools themselves). This prevents us from putting anything > >>> in, say, osd_types.h (which is included in client code) in a > >>> mempool. Which includes things like pg_log_t, a major consumer of > memory. > >>> > >>> We have a few options... > >>> > >>> - Segregate types into different compilation units so that things > >>> that are server-side only can be built separately (and put in mempools). > >>> > >>> - Just avoid these types :( > >>> > >>> - Use mempools for client code as well. > >> +1 > >> > >>> The last one is the most appealing to me, but it's violating a big > >>> no-no of shared libraries (static state!). However, I think that > >>> mempools may be one of those special things that all of the usual > >>> reasons that global variables are bad don't really apply to. The > >>> main one being: if we have multiple users of, say, librados in the > >>> same process, it is totally fine (and possibly desired?) that they > >>> would share the > >> same mempools. > >>> What do you think? > >>> > >>> FWIW, there is one bit of potentially shady initialization going on > >>> with the mempools. Instead of statically defining the pools, we > >>> instead declare an array of pool pointers, and rely on the compiler > >>> and runtime environment to zero that for the process *before* any > >>> constructors are run for any compilation unit. This allows pools to > >>> be allocated on-demand when needed by constructors in any > >>> compilation unit, regardless of the link order relative to > >>> mempool.cc. This appears to be a safe thing to do (and avoids the > >>> very tedious process of ensuring that mempool.cc is always linked first > so that it's ctors run first). > >> I don't think is a cause for concern. There are hundreds of thousands > >> of lines of existing code that are making the same assumption (global > >> statics are set to zero unless overridden explicitly). > >> > >>> If we want to take the plunge and make this usable by librados and > >>> other client code, I think it may just work, modulo leaking the > >>> pool_t structures if the .so is ever unloaded (not sure any librados > >>> user does this?). In that case, we could probably move the > >>> setup/teardown into the init/fini .so methods, although we'd need to > >>> confirm that these would run > >>> *before* any constructors for statically linked modules. > >>> > >>> Anyway.. thoughts? > >>> > >>> sage > >> -- > >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" > >> in the body of a message to majordomo@xxxxxxxxxxxxxxx More > majordomo > >> info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe ceph-devel" > > in the body of a message to majordomo@xxxxxxxxxxxxxxx More > majordomo > > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
