From: Souradeep Chakrabarti schakrabarti@linux.microsoft.com

This patch addresses the VF unload issue, where mana_dealloc_queues() gets stuck in infinite while loop, because of host unresponsiveness. It adds a timeout in the while loop, to fix it.

Fixes: ca9c54d2d6a5ab2430c4eda364c77125d62e5e0f (net: mana: Add a driver for Microsoft Azure Network Adapter) Signed-off-by: Souradeep Chakrabarti schakrabarti@linux.microsoft.com --- V2 -> V3: * Splitted the patch in two parts. * Removed the unnecessary braces from mana_dealloc_queues(). --- drivers/net/ethernet/microsoft/mana/mana_en.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c index d907727c7b7a..cb5c43c3c47e 100644 --- a/drivers/net/ethernet/microsoft/mana/mana_en.c +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c @@ -2329,7 +2329,10 @@ static int mana_dealloc_queues(struct net_device *ndev) { struct mana_port_context *apc = netdev_priv(ndev); struct gdma_dev *gd = apc->ac->gdma_dev; + unsigned long timeout; struct mana_txq *txq; + struct sk_buff *skb; + struct mana_cq *cq; int i, err;

if (apc->port_is_up) @@ -2348,13 +2351,25 @@ static int mana_dealloc_queues(struct net_device *ndev) * * Drain all the in-flight TX packets */ + + timeout = jiffies + 120 * HZ; for (i = 0; i < apc->num_queues; i++) { txq = &apc->tx_qp[i].txq; - - while (atomic_read(&txq->pending_sends) > 0) + while (atomic_read(&txq->pending_sends) > 0 && + time_before(jiffies, timeout)) usleep_range(1000, 2000); }

+ for (i = 0; i < apc->num_queues; i++) { + txq = &apc->tx_qp[i].txq; + cq = &apc->tx_qp[i].tx_cq; + while (atomic_read(&txq->pending_sends)) { + skb = skb_dequeue(&txq->pending_skbs); + mana_unmap_skb(skb, apc); + napi_consume_skb(skb, cq->budget); + atomic_sub(1, &txq->pending_sends); + } + } /* We're 100% sure the queues can no longer be woken up, because * we're sure now mana_poll_tx_cq() can't be running. */

On 6/26/2023 2:48 PM, souradeep chakrabarti wrote:

From: Souradeep Chakrabarti schakrabarti@linux.microsoft.com

This patch addresses the VF unload issue, where mana_dealloc_queues() gets stuck in infinite while loop, because of host unresponsiveness. It adds a timeout in the while loop, to fix it.

Fixes: ca9c54d2d6a5ab2430c4eda364c77125d62e5e0f (net: mana: Add a driver for Microsoft Azure Network Adapter) Signed-off-by: Souradeep Chakrabarti schakrabarti@linux.microsoft.com

---

V2 -> V3:

• Splitted the patch in two parts.
• Removed the unnecessary braces from mana_dealloc_queues().

---

drivers/net/ethernet/microsoft/mana/mana_en.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c index d907727c7b7a..cb5c43c3c47e 100644 --- a/drivers/net/ethernet/microsoft/mana/mana_en.c +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c @@ -2329,7 +2329,10 @@ static int mana_dealloc_queues(struct net_device *ndev) { struct mana_port_context *apc = netdev_priv(ndev); struct gdma_dev *gd = apc->ac->gdma_dev;

• unsigned long timeout; struct mana_txq *txq;
• struct sk_buff *skb;
• struct mana_cq *cq; int i, err;

if (apc->port_is_up) @@ -2348,13 +2351,25 @@ static int mana_dealloc_queues(struct net_device *ndev) * * Drain all the in-flight TX packets */

• timeout = jiffies + 120 * HZ; for (i = 0; i < apc->num_queues; i++) { txq = &apc->tx_qp[i].txq;

• while (atomic_read(&txq->pending_sends) > 0)
  

• while (atomic_read(&txq->pending_sends) > 0 &&
  

•        time_before(jiffies, timeout))
  usleep_range(1000, 2000);
  

  }

• for (i = 0; i < apc->num_queues; i++) {

• txq = &apc->tx_qp[i].txq;
  

• cq = &apc->tx_qp[i].tx_cq;
  

• while (atomic_read(&txq->pending_sends)) {
  

• 	skb = skb_dequeue(&txq->pending_skbs);
  

• 	mana_unmap_skb(skb, apc);
  

• 	napi_consume_skb(skb, cq->budget);
  

• 	atomic_sub(1, &txq->pending_sends);
  

• }
  

• }

Can we combine these 2 loops into 1 something like this ?

for (i = 0; i < apc->num_queues; i++) { txq = &apc->tx_qp[i].txq; cq = &apc->tx_qp[i].tx_cq; while (atomic_read(&txq->pending_sends)) { if (time_before(jiffies, timeout)) { usleep_range(1000, 2000); } else { skb = skb_dequeue(&txq->pending_skbs); mana_unmap_skb(skb, apc); napi_consume_skb(skb, cq->budget); atomic_sub(1, &txq->pending_sends); } } }

/* We're 100% sure the queues can no longer be woken up, because * we're sure now mana_poll_tx_cq() can't be running. */

From: souradeep chakrabarti schakrabarti@linux.microsoft.com Sent: Monday, June 26, 2023 2:19 AM

This patch addresses the VF unload issue, where mana_dealloc_queues() gets stuck in infinite while loop, because of host unresponsiveness. It adds a timeout in the while loop, to fix it.

For a patch series, the cover letter (patch 0 of the series) does not get included in the commit log anywhere. The cover letter can provide overall motivation and describe how the patches fit together, but the commit message for each patch should be as self-contained as possible.

The commit message here refers to "the VF unload issue", and there's no context for understanding what that issue is, though you do provide some description in the text following "where". Could you provide a commit message that is a bit more self-contained?

Same comment applies to commit message for the 2nd patch of this series.

Also, avoid text like "this patch". See the "Describe your changes" section in Documentation/process/submitting-patches.rst where the use of imperative mood is mentioned. If you like, I can provide some offline help on writing a good commit message.

Michael

Fixes: ca9c54d2d6a5ab2430c4eda364c77125d62e5e0f (net: mana: Add a driver for Microsoft Azure Network Adapter) Signed-off-by: Souradeep Chakrabarti schakrabarti@linux.microsoft.com

---

V2 -> V3:

• Splitted the patch in two parts.
• Removed the unnecessary braces from mana_dealloc_queues().

---

drivers/net/ethernet/microsoft/mana/mana_en.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c index d907727c7b7a..cb5c43c3c47e 100644 --- a/drivers/net/ethernet/microsoft/mana/mana_en.c +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c @@ -2329,7 +2329,10 @@ static int mana_dealloc_queues(struct net_device *ndev) { struct mana_port_context *apc = netdev_priv(ndev); struct gdma_dev *gd = apc->ac->gdma_dev;

• unsigned long timeout; struct mana_txq *txq;

• struct sk_buff *skb;

• struct mana_cq *cq; int i, err;

  if (apc->port_is_up)

@@ -2348,13 +2351,25 @@ static int mana_dealloc_queues(struct net_device *ndev) * * Drain all the in-flight TX packets */

• timeout = jiffies + 120 * HZ; for (i = 0; i < apc->num_queues; i++) { txq = &apc->tx_qp[i].txq;

• while (atomic_read(&txq->pending_sends) > 0)
  

• while (atomic_read(&txq->pending_sends) > 0 &&
  

•        time_before(jiffies, timeout))
  usleep_range(1000, 2000);
  

  }

• for (i = 0; i < apc->num_queues; i++) {

• txq = &apc->tx_qp[i].txq;
  

• cq = &apc->tx_qp[i].tx_cq;
  

• while (atomic_read(&txq->pending_sends)) {
  

• 	skb = skb_dequeue(&txq->pending_skbs);
  

• 	mana_unmap_skb(skb, apc);
  

• 	napi_consume_skb(skb, cq->budget);
  

• 	atomic_sub(1, &txq->pending_sends);
  

• }
  

• } /* We're 100% sure the queues can no longer be woken up, because

  • we're sure now mana_poll_tx_cq() can't be running.

  */

-- 2.34.1

From: Souradeep Chakrabarti schakrabarti@linux.microsoft.com

This is the second part of the fix.

Also this patch adds a new attribute in mana_context, which gets set when mana_hwc_send_request() hits a timeout because of host unresponsiveness. This flag then helps to avoid the timeouts in successive calls.

Fixes: ca9c54d2d6a5ab2430c4eda364c77125d62e5e0f (net: mana: Add a driver for Microsoft Azure Network Adapter) Signed-off-by: Souradeep Chakrabarti schakrabarti@linux.microsoft.com --- V2 -> V3: * Removed the initialization of vf_unload_timeout * Splitted the patch in two. * Fixed extra space from the commit message. --- drivers/net/ethernet/microsoft/mana/gdma_main.c | 4 +++- drivers/net/ethernet/microsoft/mana/hw_channel.c | 12 +++++++++++- include/net/mana/mana.h | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c index 8f3f78b68592..6411f01be0d9 100644 --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c @@ -946,10 +946,12 @@ int mana_gd_deregister_device(struct gdma_dev *gd) struct gdma_context *gc = gd->gdma_context; struct gdma_general_resp resp = {}; struct gdma_general_req req = {}; + struct mana_context *ac; int err;

if (gd->pdid == INVALID_PDID) return -EINVAL; + ac = gd->driver_data;

mana_gd_init_req_hdr(&req.hdr, GDMA_DEREGISTER_DEVICE, sizeof(req), sizeof(resp)); @@ -957,7 +959,7 @@ int mana_gd_deregister_device(struct gdma_dev *gd) req.hdr.dev_id = gd->dev_id;

err = mana_gd_send_request(gc, sizeof(req), &req, sizeof(resp), &resp); - if (err || resp.hdr.status) { + if ((err || resp.hdr.status) && !ac->vf_unload_timeout) { dev_err(gc->dev, "Failed to deregister device: %d, 0x%x\n", err, resp.hdr.status); if (!err) diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c index 9d1507eba5b9..492cb2c6e2cb 100644 --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c @@ -1,8 +1,10 @@ // SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause /* Copyright (c) 2021, Microsoft Corporation. */

+#include "asm-generic/errno.h" #include <net/mana/gdma.h> #include <net/mana/hw_channel.h> +#include <net/mana/mana.h>

static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id) { @@ -786,12 +788,19 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len, struct hwc_wq *txq = hwc->txq; struct gdma_req_hdr *req_msg; struct hwc_caller_ctx *ctx; + struct mana_context *ac; u32 dest_vrcq = 0; u32 dest_vrq = 0; u16 msg_id; int err;

mana_hwc_get_msg_index(hwc, &msg_id); + ac = hwc->gdma_dev->driver_data; + if (ac->vf_unload_timeout) { + dev_err(hwc->dev, "HWC: vport is already unloaded.\n"); + err = -ETIMEDOUT; + goto out; + }

tx_wr = &txq->msg_buf->reqs[msg_id];

@@ -825,9 +834,10 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len, goto out; }

- if (!wait_for_completion_timeout(&ctx->comp_event, 30 * HZ)) { + if (!wait_for_completion_timeout(&ctx->comp_event, 5 * HZ)) { dev_err(hwc->dev, "HWC: Request timed out!\n"); err = -ETIMEDOUT; + ac->vf_unload_timeout = true; goto out; }

diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h index 9eef19972845..5f5affdca1eb 100644 --- a/include/net/mana/mana.h +++ b/include/net/mana/mana.h @@ -358,6 +358,8 @@ struct mana_context {

u16 num_ports;

+ bool vf_unload_timeout; + struct mana_eq *eqs;

struct net_device *ports[MAX_PORTS_IN_MANA_DEV];

-----Original Message----- From: Praveen Kumar kumarpraveen@linux.microsoft.com Sent: Monday, June 26, 2023 10:13 AM To: souradeep chakrabarti schakrabarti@linux.microsoft.com; KY Srinivasan kys@microsoft.com; Haiyang Zhang haiyangz@microsoft.com; wei.liu@kernel.org; Dexuan Cui decui@microsoft.com; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Long Li longli@microsoft.com; Ajay Sharma sharmaajay@microsoft.com; leon@kernel.org; cai.huoqing@linux.dev; ssengar@linux.microsoft.com; vkuznets@redhat.com; tglx@linutronix.de; linux- hyperv@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-rdma@vger.kernel.org Cc: stable@vger.kernel.org; Souradeep Chakrabarti schakrabarti@microsoft.com Subject: Re: [PATCH 2/2 V3 net] net: mana: Fix MANA VF unload when host is unresponsive

On 6/26/2023 2:50 PM, souradeep chakrabarti wrote:

...

From: Souradeep Chakrabarti schakrabarti@linux.microsoft.com

This is the second part of the fix.

Also this patch adds a new attribute in mana_context, which gets set when mana_hwc_send_request() hits a timeout because of host unresponsiveness. This flag then helps to avoid the timeouts in successive calls.

Fixes: ca9c54d2d6a5ab2430c4eda364c77125d62e5e0f (net: mana: Add a

driver for

...

Microsoft Azure Network Adapter) Signed-off-by: Souradeep Chakrabarti schakrabarti@linux.microsoft.com

---

V2 -> V3:

• Removed the initialization of vf_unload_timeout
• Splitted the patch in two.
• Fixed extra space from the commit message.

---

drivers/net/ethernet/microsoft/mana/gdma_main.c | 4 +++- drivers/net/ethernet/microsoft/mana/hw_channel.c | 12 +++++++++++- include/net/mana/mana.h | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c

b/drivers/net/ethernet/microsoft/mana/gdma_main.c

...

index 8f3f78b68592..6411f01be0d9 100644 --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c @@ -946,10 +946,12 @@ int mana_gd_deregister_device(struct gdma_dev

*gd)

...

struct gdma_context *gc = gd->gdma_context; struct gdma_general_resp resp = {}; struct gdma_general_req req = {};

• struct mana_context *ac; int err;

  if (gd->pdid == INVALID_PDID) return -EINVAL;

• ac = gd->driver_data;

  mana_gd_init_req_hdr(&req.hdr, GDMA_DEREGISTER_DEVICE,

sizeof(req),

...

	     sizeof(resp));

@@ -957,7 +959,7 @@ int mana_gd_deregister_device(struct gdma_dev *gd) req.hdr.dev_id = gd->dev_id;

err = mana_gd_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);

• if (err || resp.hdr.status) {

• if ((err || resp.hdr.status) && !ac->vf_unload_timeout) { dev_err(gc->dev, "Failed to deregister device: %d, 0x%x\n", err, resp.hdr.status);

With !ac->vf_unload_timeout option, this message may not be correctly showing err, status. Probably you want to add explicit information during timeouts so that it give right information ? Or have the err, status field properly updated.

...

if (!err)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c

b/drivers/net/ethernet/microsoft/mana/hw_channel.c

...

index 9d1507eba5b9..492cb2c6e2cb 100644 --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c @@ -1,8 +1,10 @@ // SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause /* Copyright (c) 2021, Microsoft Corporation. */

+#include "asm-generic/errno.h" #include <net/mana/gdma.h> #include <net/mana/hw_channel.h> +#include <net/mana/mana.h>

static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16

*msg_id)

...

{ @@ -786,12 +788,19 @@ int mana_hwc_send_request(struct

hw_channel_context *hwc, u32 req_len,

...

struct hwc_wq *txq = hwc->txq; struct gdma_req_hdr *req_msg; struct hwc_caller_ctx *ctx;

• struct mana_context *ac; u32 dest_vrcq = 0; u32 dest_vrq = 0; u16 msg_id; int err;

  mana_hwc_get_msg_index(hwc, &msg_id);

• ac = hwc->gdma_dev->driver_data;

Is there a case where gdma_dev be invalid here ? If so, lets check the state and then proceed further ?

Yes, hwc->gdma_dev is assigned shortly after it's allocated - see the code below. So it's valid. But hwc->gdma_dev->driver_data is hwc, not "mana_context *ac". There are two gdma_dev in gdma_context: hwc & mana.

You can get ac from: hwc->gdma_dev->gdma_context->mana.driver_data Or, to avoid too many pointer deference, I suggest to put the vf_unload_timeout into gdma_context.

int mana_hwc_create_channel(struct gdma_context *gc) { hwc = kzalloc(sizeof(*hwc), GFP_KERNEL); ... gd->gdma_context = gc; gd->driver_data = hwc; hwc->gdma_dev = gd; hwc->dev = gc->dev;

Also, mana_gd_send_request/mana_hwc_send_request() is used in many places, not just unloading. Should you use timeout value 5 sec, and the vf_unload_timeout flag in unloading path only, and avoid touching other code paths? Please check with hostnet team for suggestions.

If we decide to let the vf_unload_timeout flag affect all code paths, not just unloading, then it should be renamed to hwc_timeout, and submit the second patch separately. If just use it for unloading, since mana_gd_deregister_device() is used by PF too, name it like: unload_hwc_timeout.

Thanks, -Haiyang