Re: [RFC PATCH 2/2] netconsole: Defer netpoll cleanup to avoid lock release during list traversal

[Breno Leitao]

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