Re: [PATCH 5/7] ibmvnic: use a lock to serialize remove/reset

Jakub Kicinski [kuba@kernel.org] wrote:

On Thu,  7 Jan 2021 23:12:34 -0800 Sukadev Bhattiprolu wrote:

quoted

Use a separate lock to serialze ibmvnic_reset() and ibmvnic_remove()
functions. ibmvnic_reset() schedules work for the worker thread and
ibmvnic_remove() flushes the work before removing the adapter. We
don't want any work to be scheduled once we start removing the
adapter (i.e after we have already flushed the work).

Locking based on functions, not on data being accessed is questionable
IMO. If you don't want work to be scheduled isn't it enough to have a
bit / flag that you set to let other flows know not to schedule reset?

Maybe I could improve the description, but the "data" being protected
is the work queue. Basically don't add to the work queue while/after
it is (being) flushed.

Existing code is checking for the VNIC_REMOVING state before scheduling
the work but without a lock. If state goes to REMOVING after we check,
we could schedule work after the flush?

quoted

@@ -5459,6 +5464,7 @@ static int ibmvnic_remove(struct vio_dev *dev)
 {
 	struct net_device *netdev = dev_get_drvdata(&dev->dev);
 	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
+	unsigned long rmflags;
 	unsigned long flags;
 
 	spin_lock_irqsave(&adapter->state_lock, flags);

@@ -5467,7 +5473,15 @@ static int ibmvnic_remove(struct vio_dev *dev)
 		return -EBUSY;
 	}

quoted

+	spin_lock_irqsave(&adapter->remove_lock, rmflags);

You can just use flags again, no need for separate variables.

Ok.

quoted

 	adapter->state = VNIC_REMOVING;
+	spin_unlock_irqrestore(&adapter->remove_lock, rmflags);