Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] ipv4: fix a RCU-list lock in fib_triestat_seq_show
To:     Qian Cai <cai@lca.pw>, Eric Dumazet <eric.dumazet@gmail.com>
Cc:     davem@davemloft.net, kuznet@ms2.inr.ac.ru, yoshfuji@linux-ipv6.org,
        kuba@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <5e2ed86a-23bc-d3e5-05ad-4e7ed147539c@gmail.com>
 <92C7474D-4592-44BF-B0ED-26253196511E@lca.pw>
From:   Eric Dumazet <eric.dumazet@gmail.com>
Message-ID: <8ae16be2-9c64-245e-0997-805f48078432@gmail.com>
Date:   Wed, 25 Mar 2020 11:31:27 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <92C7474D-4592-44BF-B0ED-26253196511E@lca.pw>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 3/25/20 10:34 AM, Qian Cai wrote:
> 
> 
>> On Mar 25, 2020, at 12:13 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>> I would prefer :
>>
>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>> index ff0c24371e3309b3068980f46d1ed743337d2a3e..4b98ffb27136d3b43f179d6b1b42fe84586acc06 100644
>> --- a/net/ipv4/fib_trie.c
>> +++ b/net/ipv4/fib_trie.c
>> @@ -2581,6 +2581,7 @@ static int fib_triestat_seq_show(struct seq_file *seq, void *v)
>>                struct hlist_head *head = &net->ipv4.fib_table_hash[h];
>>                struct fib_table *tb;
>>
>> +               rcu_read_lock();
>>                hlist_for_each_entry_rcu(tb, head, tb_hlist) {
>>                        struct trie *t = (struct trie *) tb->tb_data;
>>                        struct trie_stat stat;
>> @@ -2596,6 +2597,7 @@ static int fib_triestat_seq_show(struct seq_file *seq, void *v)
>>                        trie_show_usage(seq, t->stats);
>> #endif
>>                }
>> +               rcu_read_unlock();
>>        }
>>
>>        return 0;
> 
> I have no strong opinion either way. My initial thought was to save 255 extra lock/unlock with a single lock/unlock, but I am not sure how time-consuming for each iteration of the outer loop could be. If it could take a bit too long, it does make a lot of sense to reduce the critical section.
> 


This file could be quite big in some setups.

Alternatively you could use cond_resched_rcu()