On Wed, Sep 6, 2017 at 1:33 PM, Jiri Pirko <j...@resnulli.us> wrote: > Wed, Sep 06, 2017 at 07:40:02PM CEST, xiyou.wangc...@gmail.com wrote: >>On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko <j...@resnulli.us> wrote: >>> From: Jiri Pirko <j...@mellanox.com> >>> >>> There's a memleak happening for chain 0. The thing is, chain 0 needs to >>> be always present, not created on demand. Therefore tcf_block_get upon >>> creation of block calls the tcf_chain_create function directly. The >>> chain is created with refcnt == 1, which is not correct in this case and >>> causes the memleak. So move the refcnt increment into tcf_chain_get >>> function even for the case when chain needs to be created. >>> >> >>Your approach could work but you just make the code even >>uglier than it is now: >> >>1. The current code is already ugly for special-casing chain 0: >> >> if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0) >> tcf_chain_destroy(chain); >> >>2. With your patch, chain 0 has a different _initial_ refcnt with others. > > No. Initial refcnt is the same. ! for every action that holds the chain. > So actually, it returns it back where it should be.
Not all all. tcf_block_get() calls tcf_chain_create(, 0), after your patch chain 0 has refcnt==0 initially. Non-0 chain? They are created via tcf_chain_get(), aka, refcnt==0 initially. > > >> >>3. Allowing an object (chain 0) exists with refcnt==0 > > So? That is for every chain that does not have goto_chain action > pointing at. Please read the code. So you are pretending to be GC but you are apparently not. You create all the troubles by setting yourself to believe chain 0 is special and refcnt==0 is okay. Both are wrong. Actually the !list_empty() check is totally unnecessary too, it is yet another place you get it wrong, you hide the race condition in commit 744a4cf63e52 which makes it harder to expose. I understand you don't trust me. Look at DaveM's reaction to your refcnt==0 madness. Remember, refcnt can be very simple, you just want to make it harder by abusing it or attempting to invent a GC. I am going to update my patch (to remove all your madness) since this is horribly wrong to me. Sorry.
