From: ljp <ljp@linux.vnet.ibm.com>
To: netdev@vger.kernel.org
Subject: Re: [PATCH v2 1/1] powerpc/vnic: Extend "failover pending" window
Date: Tue, 27 Oct 2020 17:32:39 -0500 [thread overview]
Message-ID: <34e618d28741ae5bac8cb7e126f79f65@linux.vnet.ibm.com> (raw)
In-Reply-To: <20201024000233.GC1411079@us.ibm.com>
On 2020-10-23 19:02, Sukadev Bhattiprolu wrote:
> Lijun Pan [ljp@linux.vnet.ibm.com] wrote:
>> On Oct 19, 2020, at 2:52 PM, Sukadev Bhattiprolu
>> <[1]sukadev@linux.ibm.com> wrote:
>>
>> From 67f8977f636e462a1cd1eadb28edd98ef4f2b756 Mon Sep 17 00:00:00
>> 2001
>> From: Sukadev Bhattiprolu <[2]sukadev@linux.vnet.ibm.com>
>> Date: Thu, 10 Sep 2020 11:18:41 -0700
>> Subject: [PATCH 1/1] powerpc/vnic: Extend "failover pending" window
>> Commit 5a18e1e0c193b introduced the 'failover_pending' state to
>> track
>> the "failover pending window" - where we wait for the partner to
>> become
>> ready (after a transport event) before actually attempting to
>> failover.
>> i.e window is between following two events:
>> a. we get a transport event due to a FAILOVER
>> b. later, we get CRQ_INITIALIZED indicating the partner is
>> ready at which point we schedule a FAILOVER reset.
>> and ->failover_pending is true during this window.
>> If during this window, we attempt to open (or close) a device, we
>> pretend
>> that the operation succeded and let the FAILOVER reset path
>> complete
>> the
>> operation.
>> This is fine, except if the transport event ("a" above) occurs
>> during
>> the
>> open and after open has already checked whether a failover is
>> pending.
>> If
>> that happens, we fail the open, which can cause the boot scripts to
>> leave
>> the interface down requiring administrator to manually bring up the
>> device.
>> This fix "extends" the failover pending window till we are
>> _actually_
>> ready to perform the failover reset (i.e until after we get the
>> RTNL
>> lock). Since open() holds the RTNL lock, we can be sure that we
>> either
>> finish the open or if the open() fails due to the failover pending
>> window,
>> we can again pretend that open is done and let the failover
>> complete
>> it.
>> Signed-off-by: Sukadev Bhattiprolu <[3]sukadev@linux.ibm.com>
>> ---
>> Changelog [v2]:
>> [Brian King] Ensure we clear failover_pending during hard reset
>> ---
>> drivers/net/ethernet/ibm/ibmvnic.c | 36
>> ++++++++++++++++++++++++++----
>> 1 file changed, 32 insertions(+), 4 deletions(-)
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
>> b/drivers/net/ethernet/ibm/ibmvnic.c
>> index 1b702a43a5d0..2a0f6f6820db 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -1197,18 +1197,27 @@ static int ibmvnic_open(struct net_device
>> *netdev)
>> if (adapter->state != VNIC_CLOSED) {
>> rc = ibmvnic_login(netdev);
>> if (rc)
>> - return rc;
>> + goto out;
>> rc = init_resources(adapter);
>> if (rc) {
>> netdev_err(netdev, "failed to initialize resources\n");
>> release_resources(adapter);
>> - return rc;
>> + goto out;
>> }
>> }
>> rc = __ibmvnic_open(netdev);
>> +out:
>> + /*
>> + * If open fails due to a pending failover, set device state and
>> + * return. Device operation will be handled by reset routine.
>> + */
>> + if (rc && adapter->failover_pending) {
>> + adapter->state = VNIC_OPEN;
>> + rc = 0;
>> + }
>> return rc;
>> }
>> @@ -1931,6 +1940,13 @@ static int do_reset(struct ibmvnic_adapter
>> *adapter,
>> rwi->reset_reason);
>> rtnl_lock();
>> + /*
>> + * Now that we have the rtnl lock, clear any pending failover.
>> + * This will ensure ibmvnic_open() has either completed or will
>> + * block until failover is complete.
>> + */
>> + if (rwi->reset_reason == VNIC_RESET_FAILOVER)
>> + adapter->failover_pending = false;
>> netif_carrier_off(netdev);
>> adapter->reset_reason = rwi->reset_reason;
>> @@ -2211,6 +2227,13 @@ static void __ibmvnic_reset(struct
>> work_struct
>> *work)
>> /* CHANGE_PARAM requestor holds rtnl_lock */
>> rc = do_change_param_reset(adapter, rwi, reset_state);
>> } else if (adapter->force_reset_recovery) {
>> + /*
>> + * Since we are doing a hard reset now, clear the
>> + * failover_pending flag so we don't ignore any
>> + * future MOBILITY or other resets.
>> + */
>> + adapter->failover_pending = false;
>> +
>>
>> I think it would be better to put above chunk of code to
>> do_hard_reset()
>> like you do for do_reset(), if you really want to extend the
>> window
>
> I put it here because we clear the other flags like
> force_reset_recovery
> also. I have been considering moving the check ->wait_for_reset and the
> rtnl lock also into do_hard_reset(). I will queue that reorg separate
> from this bug fix.
>
>> this way.
>> Extending the window that long may cause some resets being
>> skipped in some scenarios though I don’t know yet.
>
> Yes hard to prove, but we have run several tests on this and seems to
> be working.
>
>> We have already seen the migration reset being skipped in some
>> cases.
>
> Is that happening due to this patch or in general? If a migration
> occurs
This patch changed the original behavior and clears failover_pending
only in do_reset which caused migration reset being skipped in the
scenario like,
do_rest(migration reset) somehow calls into do_hard_reset(failover
reset),
and the failover_pending is not cleared, so that do_hard_reset is
skipped,
and migration reset is not completely completed.
This is why Brian King suggested this to fix the regression of v1 patch.
Changelog [v2]:
[Brian King] Ensure we clear failover_pending during hard reset
> while failover is pending, we should review the best way to handle
> that.
> Will failover complete in such a case or should we punt the failover
> and
> process migration instead? I think that should be addressed separately
> because that window is smaller but is there even without this patch?
>
>> So my point is extending the window is kind of risky, and do we
>> have an
>> alternative to address the "open” problem you want to solve
>> originally?
>> For example, would it be a viable approach to only change the code
>> in
>> ibmvnic_open() or __ibmvnic_open(), but not extend this window?
>
> Not sure. We could try and block the open until failover is completed
> but a) that could still timeout the application and b) Existing code
> "pretends" that failover occurred "just after" open succeeded, so marks
> the open successful and lets the failover complete the open.
>
> Besides, we should also not assume that the failover window will be
> short right?
>
>>
>> /* Transport event occurred during previous reset */
>> if (adapter->wait_for_reset) {
>> /* Previous was CHANGE_PARAM; caller locked */
>> @@ -2275,9 +2298,15 @@ static int ibmvnic_reset(struct
>> ibmvnic_adapter
>> *adapter,
>> unsigned long flags;
>> int ret;
>> + /*
>> + * If failover is pending don't schedule any other reset.
>> + * Instead let the failover complete. If there is already a
>> + * a failover reset scheduled, we will detect and drop the
>> + * duplicate reset when walking the ->rwi_list below.
>> + */
>> if (adapter->state == VNIC_REMOVING ||
>> adapter->state == VNIC_REMOVED ||
>> - adapter->failover_pending) {
>> + (adapter->failover_pending && reason != VNIC_RESET_FAILOVER))
>> {
>> ret = EBUSY;
>> netdev_dbg(netdev, "Adapter removing or pending failover, skipping
>> reset\n");
>> goto err;
>> @@ -4653,7 +4682,6 @@ static void ibmvnic_handle_crq(union
>> ibmvnic_crq
>> *crq,
>> case IBMVNIC_CRQ_INIT:
>> dev_info(dev, "Partner initialized\n");
>> adapter->from_passive_init = true;
>> - adapter->failover_pending = false;
>> if (!completion_done(&adapter->init_done)) {
>> complete(&adapter->init_done);
>> adapter->init_done_rc = -EIO;
>> --
>> 2.25.4
>>
>> References
>>
>> 1. mailto:sukadev@linux.ibm.com
>> 2. mailto:sukadev@linux.vnet.ibm.com
>> 3. mailto:sukadev@linux.ibm.com
next prev parent reply other threads:[~2020-10-27 22:32 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-19 19:52 [PATCH v2 1/1] powerpc/vnic: Extend "failover pending" window Sukadev Bhattiprolu
2020-10-21 1:25 ` Jakub Kicinski
[not found] ` <FC9128E6-0E4A-476C-8A98-B04785385841@linux.vnet.ibm.com>
2020-10-24 0:02 ` Sukadev Bhattiprolu
2020-10-27 22:32 ` ljp [this message]
2020-10-30 4:00 ` ljp
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=34e618d28741ae5bac8cb7e126f79f65@linux.vnet.ibm.com \
--to=ljp@linux.vnet.ibm.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).