| Message ID | 20190917171552.32498-3-julietk@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net/ibmvnic: serialization fixes | expand |
Hi Juliet, Juliet Kim <julietk@linux.vnet.ibm.com> writes: > Signed-off-by: Juliet Kim <julietk@linux.vnet.ibm.com> > --- > drivers/net/ethernet/ibm/ibmvnic.c | 23 ++++++++++++++++++++++- > drivers/net/ethernet/ibm/ibmvnic.h | 3 +++ > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c > index ba340aaff1b3..f344ccd68ad9 100644 > --- a/drivers/net/ethernet/ibm/ibmvnic.c > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > @@ -2054,6 +2054,13 @@ static void __ibmvnic_reset(struct work_struct *work) > > adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset); > > + if (adapter->resetting) { > + schedule_delayed_work(&adapter->ibmvnic_delayed_reset, > + IBMVNIC_RESET_DELAY); > + return; > + } > + > + adapter->resetting = true; > reset_state = adapter->state; Is there some locking/serialisation around this? Otherwise that looks very racy. ie. two CPUs could both see adapter->resetting == false, then both set it to true, and then continue executing and stomp on each other. cheers
On 9/18/19 1:12 AM, Michael Ellerman wrote: > Hi Juliet, > > Juliet Kim <julietk@linux.vnet.ibm.com> writes: >> Signed-off-by: Juliet Kim <julietk@linux.vnet.ibm.com> >> --- >> drivers/net/ethernet/ibm/ibmvnic.c | 23 ++++++++++++++++++++++- >> drivers/net/ethernet/ibm/ibmvnic.h | 3 +++ >> 2 files changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c >> index ba340aaff1b3..f344ccd68ad9 100644 >> --- a/drivers/net/ethernet/ibm/ibmvnic.c >> +++ b/drivers/net/ethernet/ibm/ibmvnic.c >> @@ -2054,6 +2054,13 @@ static void __ibmvnic_reset(struct work_struct *work) >> >> adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset); >> >> + if (adapter->resetting) { >> + schedule_delayed_work(&adapter->ibmvnic_delayed_reset, >> + IBMVNIC_RESET_DELAY); >> + return; >> + } >> + >> + adapter->resetting = true; >> reset_state = adapter->state; > Is there some locking/serialisation around this? > > Otherwise that looks very racy. ie. two CPUs could both see > adapter->resetting == false, then both set it to true, and then continue > executing and stomp on each other. > > cheers I agree there may be a race here. Thank you for reviewing. I will address it in the next version.
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index ba340aaff1b3..f344ccd68ad9 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -2054,6 +2054,13 @@ static void __ibmvnic_reset(struct work_struct *work) adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset); + if (adapter->resetting) { + schedule_delayed_work(&adapter->ibmvnic_delayed_reset, + IBMVNIC_RESET_DELAY); + return; + } + + adapter->resetting = true; reset_state = adapter->state; rwi = get_next_rwi(adapter); @@ -2095,6 +2102,10 @@ static void __ibmvnic_reset(struct work_struct *work) break; rwi = get_next_rwi(adapter); + + if (rwi && (rwi->reset_reason == VNIC_RESET_FAILOVER || + rwi->reset_reason == VNIC_RESET_MOBILITY)) + adapter->force_reset_recovery = true; } if (adapter->wait_for_reset) { @@ -2110,6 +2121,15 @@ static void __ibmvnic_reset(struct work_struct *work) adapter->resetting = false; } +static void __ibmvnic_delayed_reset(struct work_struct *work) +{ + struct ibmvnic_adapter *adapter; + + adapter = container_of(work, struct ibmvnic_adapter, + ibmvnic_delayed_reset.work); + __ibmvnic_reset(&adapter->ibmvnic_reset); +} + static int ibmvnic_reset(struct ibmvnic_adapter *adapter, enum ibmvnic_reset_reason reason) { @@ -2162,7 +2182,6 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter, rwi->reset_reason = reason; list_add_tail(&rwi->list, &adapter->rwi_list); spin_unlock_irqrestore(&adapter->rwi_lock, flags); - adapter->resetting = true; netdev_dbg(adapter->netdev, "Scheduling reset (reason %d)\n", reason); schedule_work(&adapter->ibmvnic_reset); @@ -4933,6 +4952,8 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id) spin_lock_init(&adapter->stats_lock); INIT_WORK(&adapter->ibmvnic_reset, __ibmvnic_reset); + INIT_DELAYED_WORK(&adapter->ibmvnic_delayed_reset, + __ibmvnic_delayed_reset); INIT_LIST_HEAD(&adapter->rwi_list); spin_lock_init(&adapter->rwi_lock); init_completion(&adapter->init_done); diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h index 9d3d35cc91d6..4f4651d92cc1 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.h +++ b/drivers/net/ethernet/ibm/ibmvnic.h @@ -39,6 +39,8 @@ #define IBMVNIC_MAX_LTB_SIZE ((1 << (MAX_ORDER - 1)) * PAGE_SIZE) #define IBMVNIC_BUFFER_HLEN 500 +#define IBMVNIC_RESET_DELAY 100 + static const char ibmvnic_priv_flags[][ETH_GSTRING_LEN] = { #define IBMVNIC_USE_SERVER_MAXES 0x1 "use-server-maxes" @@ -1077,6 +1079,7 @@ struct ibmvnic_adapter { spinlock_t rwi_lock; struct list_head rwi_list; struct work_struct ibmvnic_reset; + struct delayed_work ibmvnic_delayed_reset; bool resetting; bool napi_enabled, from_passive_init;
Signed-off-by: Juliet Kim <julietk@linux.vnet.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 23 ++++++++++++++++++++++- drivers/net/ethernet/ibm/ibmvnic.h | 3 +++ 2 files changed, 25 insertions(+), 1 deletion(-)