Re: [PATCH net-next v2 0/7] ibmvnic: Use more consistent locking

From: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org, Dany Madden <drt@linux.ibm.com>,
	Lijun Pan <ljp@linux.ibm.com>,
	Rick Lindsley <ricklind@linux.ibm.com>
Subject: Re: [PATCH net-next v2 0/7] ibmvnic: Use more consistent locking
Date: Wed, 13 Jan 2021 11:55:03 -0800	[thread overview]
Message-ID: <20210113195503.GA236972@us.ibm.com> (raw)
In-Reply-To: <20210113105735.20853d1f@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

Jakub Kicinski [kuba@kernel.org] wrote:
> On Tue, 12 Jan 2021 20:42:47 -0800 Sukadev Bhattiprolu wrote:
> > Jakub Kicinski [kuba@kernel.org] wrote:
> > > On Tue, 12 Jan 2021 10:14:34 -0800 Sukadev Bhattiprolu wrote:  
> > > > Use more consistent locking when reading/writing the adapter->state
> > > > field. This patch set fixes a race condition during ibmvnic_open()
> > > > where the adapter could be left in the PROBED state if a reset occurs
> > > > at the wrong time. This can cause networking to not come up during
> > > > boot and potentially require manual intervention in bringing up
> > > > applications that depend on the network.  
> > > 
> > > Apologies for not having enough time to suggest details, but let me
> > > state this again - the patches which fix bugs need to go into net with
> > > Fixes tags, the refactoring needs to go to net-next without Fixes tags.
> > > If there are dependencies, patches go to net first, then within a week
> > > or so the reset can be posted for net-next, after net -> net-next merge.  
> > 
> > Well, the patch set fixes a set of bugs - main one is a locking bug fixed
> > in patch 6. Other bugs are more minor or corner cases. Fixing the locking
> > bug requires some refactoring/simplifying/reordering checks so lock can be
> > properly acquired.
> > 
> > Because of the size/cleanup, should we treat it as "next" material? i.e
> > should I just drop the Fixes tag and resend to net-next?
> > 
> > Or can we ignore the size of patchset and treat it all as bug fixes?
> 
> No, focus on doing this right rather than trying to justify why your
> patches deserve special treatment.

I am not asking for special treatment. I just don't get the rationale
for saying that a bug fix cannot have some amount of refactoring.
Specially a bug fix like this locking one which clearly touches various
parts of the code. To take the lock properly we do have to move code
around.
> 
> Throw this entire series out and start over with the goal of fixing 
> the bugs with minimal patches.

Fixing corner case bugs that have been around for a while in code that
we are going to throw away feels like spinning wheels just to comply
with the "process".

Other than the optimization for the spin lock, there have been no
reported technical issues with the patch set. Throwing away the
patch set and starting over - I would be focusing not on the bugs
or making the driver better but somehow complying with the process.

The tiny memory leak issues I mention for completeness are just rare
corner cases and a distraction. The main issue that people actually
hit is the locking one. Fixing the locking issue touches lot of code.

I to throw away the list implementation and add a couple of simple
interfaces because if the allocation fails we call ibmvnic_close() -
that messes with the locking I am trying to fix.

Sukadev