[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Zp4qGdGk7vLJaCPs@gmail.com>
Date: Mon, 22 Jul 2024 02:44:57 -0700
From: Breno Leitao <leitao@...ian.org>
To: Rik van Riel <riel@...riel.com>
Cc: kuba@...nel.org, davem@...emloft.net, edumazet@...gle.com,
pabeni@...hat.com, thepacketgeek@...il.com, horms@...nel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
paulmck@...nel.org, davej@...emonkey.org.uk
Subject: Re: [RFC PATCH 2/2] netconsole: Defer netpoll cleanup to avoid lock
release during list traversal
Hello Rik,
On Thu, Jul 18, 2024 at 03:53:54PM -0400, Rik van Riel wrote:
> On Thu, 2024-07-18 at 11:43 -0700, Breno Leitao wrote:
> >
> > +/* Clean up every target in the cleanup_list and move the clean
> > targets back to the
> > + * main target_list.
> > + */
> > +static void netconsole_process_cleanups_core(void)
> > +{
> > + struct netconsole_target *nt, *tmp;
> > + unsigned long flags;
> > +
> > + /* The cleanup needs RTNL locked */
> > + ASSERT_RTNL();
> > +
> > + mutex_lock(&target_cleanup_list_lock);
> > + list_for_each_entry_safe(nt, tmp, &target_cleanup_list,
> > list) {
> > + /* all entries in the cleanup_list needs to be
> > disabled */
> > + WARN_ON_ONCE(nt->enabled);
> > + do_netpoll_cleanup(&nt->np);
> > + /* moved the cleaned target to target_list. Need to
> > hold both locks */
> > + spin_lock_irqsave(&target_list_lock, flags);
> > + list_move(&nt->list, &target_list);
> > + spin_unlock_irqrestore(&target_list_lock, flags);
> > + }
> > + WARN_ON_ONCE(!list_empty(&target_cleanup_list));
> > + mutex_unlock(&target_cleanup_list_lock);
> > +}
> > +
> > +/* Do the list cleanup with the rtnl lock hold */
> > +static void netconsole_process_cleanups(void)
> > +{
> > + rtnl_lock();
> > + netconsole_process_cleanups_core();
> > + rtnl_unlock();
> > +}
> >
First of all, thanks for reviewing this patch.
> I've got what may be a dumb question.
>
> If the traversal of the target_cleanup_list happens under
> the rtnl_lock, why do you need a new lock.
Because the lock protect the target_cleanup_list list, and in some
cases, the list is accessed outside of the region that holds the `rtnl`
locks.
For instance, enabled_store() is a function that is called from
user space (through confifs). This function needs to populate
target_cleanup_list (for targets that are being disabled). This
code path does NOT has rtnl at all.
> and why is there
> a wrapper function that only takes this one lock, and then
> calls the other function?
I assume that the network cleanup needs to hold rtnl, since it is going
to release a network interface. Thus, __netpoll_cleanup() needs to be
called protected by rtnl lock.
That said, netconsole calls `__netpoll_cleanup()` indirectly through 2
different code paths.
1) From enabled_store() -- userspace disabling the interface from
configfs.
* This code path does not have `rtnl` held, thus, it needs
to be held along the way.
2) From netconsole_netdev_event() -- A network event callback
* This function is called with `rtnl` held, thus, no
need to acquire it anymore.
> Are you planning a user of netconsole_process_cleanups_core()
> that already holds the rtnl_lock and should not use this
> wrapper?
In fact, this patch is already using it today. See its invocation from
netconsole_netdev_event().
> Also, the comment does not explain why the rtnl_lock is held.
> We can see that it grabs it, but not why. It would be nice to
> have that in the comment.
Agree. I will add this comment in my changes.
Thank you!
--breno
Powered by blists - more mailing lists