From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E5A56C433E0 for ; Tue, 12 Jan 2021 00:34:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BC7DB22D5B for ; Tue, 12 Jan 2021 00:34:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2405468AbhALAZf (ORCPT ); Mon, 11 Jan 2021 19:25:35 -0500 Received: from mail.kernel.org ([198.145.29.99]:36454 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2404199AbhAKXrP (ORCPT ); Mon, 11 Jan 2021 18:47:15 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id DEC5122D00; Mon, 11 Jan 2021 23:46:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1610408793; bh=LCczpMEwQ7Inf94GYXmm33lV1dDfuGjkKnAZtODSx+Q=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=ok1LVieRpA9vIX3sw76GFGJV0FCZuJhx/Y9GPfDA1yGyAfdozL4euIiH/Yr4YR5yN wPx38wYy8+PuFbmOEH8vttMizUtXhtzcWEuhsE1BJnFqr2fu9iCLVzAIGGrgReZZ91 bmjiBn6X9xcrL1dIgv8iIRG3rMpXAdPeSSgAef3MxmVQG9xIsg3QfeiQQK5UFxDchv RzbPKp3XCSIyaP55RZukMS44fstfOABZKER0egaurFDD9WZN0ri/zAIRNyG2ZwG0Ua 6+4D+eebviwIp9oj18jREmGQqZ5uO7As9y1Qddlsxrji31wUQBCkn6H044dp/QuEn1 vy4x4+Ts+boKQ== Message-ID: Subject: Re: [PATCH v6 net-next 03/15] net: procfs: hold netif_lists_lock when retrieving device statistics From: Saeed Mahameed To: Vladimir Oltean , "David S . Miller" , Jakub Kicinski Cc: netdev@vger.kernel.org, Andrew Lunn , Florian Fainelli , Cong Wang , Stephen Hemminger , Eric Dumazet , George McCollister , Oleksij Rempel , Jay Vosburgh , Veaceslav Falico , Andy Gospodarek , Arnd Bergmann , Taehee Yoo , Jiri Pirko , Florian Westphal , Nikolay Aleksandrov , Pravin B Shelar , Sridhar Samudrala Date: Mon, 11 Jan 2021 15:46:32 -0800 In-Reply-To: <20210109172624.2028156-4-olteanv@gmail.com> References: <20210109172624.2028156-1-olteanv@gmail.com> <20210109172624.2028156-4-olteanv@gmail.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, 2021-01-09 at 19:26 +0200, Vladimir Oltean wrote: > From: Vladimir Oltean > > In the effort of making .ndo_get_stats64 be able to sleep, we need to > ensure the callers of dev_get_stats do not use atomic context. > > The /proc/net/dev file uses an RCU read-side critical section to > ensure > the integrity of the list of network interfaces, because it iterates > through all net devices in the netns to show their statistics. > > To offer the equivalent protection against an interface registering > or > deregistering, while also remaining in sleepable context, we can use > the > netns mutex for the interface lists. > > Cc: Cong Wang > Cc: Eric Dumazet > Signed-off-by: Vladimir Oltean > --- > Changes in v6: > None. > > Changes in v5: > None. > > Changes in v4: > None. > > Changes in v3: > None. > > Changes in v2: > None. > > net/core/net-procfs.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c > index c714e6a9dad4..4784703c1e39 100644 > --- a/net/core/net-procfs.c > +++ b/net/core/net-procfs.c > @@ -21,7 +21,7 @@ static inline struct net_device > *dev_from_same_bucket(struct seq_file *seq, loff > unsigned int count = 0, offset = get_offset(*pos); > > h = &net->dev_index_head[get_bucket(*pos)]; > - hlist_for_each_entry_rcu(dev, h, index_hlist) { > + hlist_for_each_entry(dev, h, index_hlist) { > if (++count == offset) > return dev; > } > @@ -51,9 +51,11 @@ static inline struct net_device > *dev_from_bucket(struct seq_file *seq, loff_t *p > * in detail. > */ > static void *dev_seq_start(struct seq_file *seq, loff_t *pos) > - __acquires(RCU) > { > - rcu_read_lock(); > + struct net *net = seq_file_net(seq); > + > + netif_lists_lock(net); > + This can be very costly, holding a mutex while traversing the whole netedv lists and reading their stats, we need to at least allow multiple readers to enter as it was before, so maybe you want to use rw_semaphore instead of the mutex. or just have a unified approach of rcu+refcnt/dev_hold as you did for bonding and failover patches #13..#14, I used the same approach to achieve the same for sysfs and procfs more than 2 years ago, you are welcome to use my patches: https://lore.kernel.org/netdev/4cc44e85-cb5e-502c-30f3-c6ea564fe9ac@gmail.com/ > if (!*pos) > return SEQ_START_TOKEN; > > @@ -70,9 +72,10 @@ static void *dev_seq_next(struct seq_file *seq, > void *v, loff_t *pos) > } > > static void dev_seq_stop(struct seq_file *seq, void *v) > - __releases(RCU) > { > - rcu_read_unlock(); > + struct net *net = seq_file_net(seq); > + > + netif_lists_unlock(net); > } > > static void dev_seq_printf_stats(struct seq_file *seq, struct > net_device *dev)