| Message ID | 1690377336-1353-1-git-send-email-schakrabarti@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [V6,net] net: mana: Fix MANA VF unload when hardware is | expand |
在 2023/7/26 21:15, Souradeep Chakrabarti 写道: > When unloading the MANA driver, mana_dealloc_queues() waits for the MANA > hardware to complete any inflight packets and set the pending send count > to zero. But if the hardware has failed, mana_dealloc_queues() > could wait forever. > > Fix this by adding a timeout to the wait. Set the timeout to 120 seconds, > which is a somewhat arbitrary value that is more than long enough for > functional hardware to complete any sends. > > Cc: stable@vger.kernel.org > Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)") > > Signed-off-by: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com> > --- > V5 -> V6: > * Added pcie_flr to reset the pci after timeout. > * Fixed the position of changelog. > * Removed unused variable like cq. > > V4 -> V5: > * Added fixes tag > * Changed the usleep_range from static to incremental value. > * Initialized timeout in the begining. > > V3 -> V4: > * Removed the unnecessary braces from mana_dealloc_queues(). > > V2 -> V3: > * Removed the unnecessary braces from mana_dealloc_queues(). > > V1 -> V2: > * Added net branch > * Removed the typecasting to (struct mana_context*) of void pointer > * Repositioned timeout variable in mana_dealloc_queues() > * Repositioned vf_unload_timeout in mana_context struct, to utilise the > 6 bytes hole > --- > drivers/net/ethernet/microsoft/mana/mana_en.c | 38 +++++++++++++++++-- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c > index a499e460594b..ea039e2d4c4b 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c > @@ -8,6 +8,7 @@ > #include <linux/ethtool.h> > #include <linux/filter.h> > #include <linux/mm.h> > +#include <linux/pci.h> > > #include <net/checksum.h> > #include <net/ip6_checksum.h> > @@ -2345,9 +2346,12 @@ int mana_attach(struct net_device *ndev) > static int mana_dealloc_queues(struct net_device *ndev) > { > struct mana_port_context *apc = netdev_priv(ndev); > + unsigned long timeout = jiffies + 120 * HZ; > struct gdma_dev *gd = apc->ac->gdma_dev; > struct mana_txq *txq; > + struct sk_buff *skb; > int i, err; > + u32 tsleep; > > if (apc->port_is_up) > return -EINVAL; > @@ -2363,15 +2367,41 @@ static int mana_dealloc_queues(struct net_device *ndev) > * to false, but it doesn't matter since mana_start_xmit() drops any > * new packets due to apc->port_is_up being false. > * > - * Drain all the in-flight TX packets > + * Drain all the in-flight TX packets. > + * A timeout of 120 seconds for all the queues is used. > + * This will break the while loop when h/w is not responding. > + * This value of 120 has been decided here considering max > + * number of queues. > */ > + > for (i = 0; i < apc->num_queues; i++) { > txq = &apc->tx_qp[i].txq; > - > - while (atomic_read(&txq->pending_sends) > 0) > - usleep_range(1000, 2000); > + tsleep = 1000; > + while (atomic_read(&txq->pending_sends) > 0 && > + time_before(jiffies, timeout)) { > + usleep_range(tsleep, tsleep + 1000); > + tsleep <<= 1; > + } > + if (atomic_read(&txq->pending_sends)) { > + err = pcie_flr(to_pci_dev(gd->gdma_context->dev)); > + if (err) { > + netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n", > + err, atomic_read(&txq->pending_sends), > + txq->gdma_txq_id); > + } > + break; > + } > } > > + for (i = 0; i < apc->num_queues; i++) { > + txq = &apc->tx_qp[i].txq; > + while (atomic_read(&txq->pending_sends)) { > + skb = skb_dequeue(&txq->pending_skbs); > + mana_unmap_skb(skb, apc); > + dev_consume_skb_any(skb); > + atomic_sub(1, &txq->pending_sends); > + } If I get this commit correctly, txq->pending_sends should be equal to the length of txq->pending_skbs? If yes, can we only handle the pending_skbs? the above snippet can be changed to as below? So the performance is better? " while ((skb = skb_dequeue(&txq->pending_skbs))) { mana_unmap_skb(skb, apc); dev_consume_skb_any(skb); } atomic_set(&txq->pending_sends, 0); " Zhu Yanjun > + } > /* 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 Thu, Jul 27, 2023 at 9:07 AM Zhu Yanjun <yanjun.zhu@linux.dev> wrote: > > 在 2023/7/26 21:15, Souradeep Chakrabarti 写道: > > When unloading the MANA driver, mana_dealloc_queues() waits for the MANA > > hardware to complete any inflight packets and set the pending send count > > to zero. But if the hardware has failed, mana_dealloc_queues() > > could wait forever. > > > > Fix this by adding a timeout to the wait. Set the timeout to 120 seconds, > > which is a somewhat arbitrary value that is more than long enough for > > functional hardware to complete any sends. > > > > Cc: stable@vger.kernel.org > > Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)") > > > > Signed-off-by: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com> > > --- > > V5 -> V6: > > * Added pcie_flr to reset the pci after timeout. > > * Fixed the position of changelog. > > * Removed unused variable like cq. > > > > V4 -> V5: > > * Added fixes tag > > * Changed the usleep_range from static to incremental value. > > * Initialized timeout in the begining. > > > > V3 -> V4: > > * Removed the unnecessary braces from mana_dealloc_queues(). > > > > V2 -> V3: > > * Removed the unnecessary braces from mana_dealloc_queues(). > > > > V1 -> V2: > > * Added net branch > > * Removed the typecasting to (struct mana_context*) of void pointer > > * Repositioned timeout variable in mana_dealloc_queues() > > * Repositioned vf_unload_timeout in mana_context struct, to utilise the > > 6 bytes hole > > --- > > drivers/net/ethernet/microsoft/mana/mana_en.c | 38 +++++++++++++++++-- > > 1 file changed, 34 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c > > index a499e460594b..ea039e2d4c4b 100644 > > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c > > @@ -8,6 +8,7 @@ > > #include <linux/ethtool.h> > > #include <linux/filter.h> > > #include <linux/mm.h> > > +#include <linux/pci.h> > > > > #include <net/checksum.h> > > #include <net/ip6_checksum.h> > > @@ -2345,9 +2346,12 @@ int mana_attach(struct net_device *ndev) > > static int mana_dealloc_queues(struct net_device *ndev) > > { > > struct mana_port_context *apc = netdev_priv(ndev); > > + unsigned long timeout = jiffies + 120 * HZ; > > struct gdma_dev *gd = apc->ac->gdma_dev; > > struct mana_txq *txq; > > + struct sk_buff *skb; > > int i, err; > > + u32 tsleep; > > > > if (apc->port_is_up) > > return -EINVAL; > > @@ -2363,15 +2367,41 @@ static int mana_dealloc_queues(struct net_device *ndev) > > * to false, but it doesn't matter since mana_start_xmit() drops any > > * new packets due to apc->port_is_up being false. > > * > > - * Drain all the in-flight TX packets > > + * Drain all the in-flight TX packets. > > + * A timeout of 120 seconds for all the queues is used. > > + * This will break the while loop when h/w is not responding. > > + * This value of 120 has been decided here considering max > > + * number of queues. > > */ > > + > > for (i = 0; i < apc->num_queues; i++) { > > txq = &apc->tx_qp[i].txq; > > - > > - while (atomic_read(&txq->pending_sends) > 0) > > - usleep_range(1000, 2000); > > + tsleep = 1000; > > + while (atomic_read(&txq->pending_sends) > 0 && > > + time_before(jiffies, timeout)) { > > + usleep_range(tsleep, tsleep + 1000); > > + tsleep <<= 1; > > + } > > + if (atomic_read(&txq->pending_sends)) { > > + err = pcie_flr(to_pci_dev(gd->gdma_context->dev)); > > + if (err) { > > + netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n", > > + err, atomic_read(&txq->pending_sends), > > + txq->gdma_txq_id); > > + } > > + break; > > + } > > } > > > > + for (i = 0; i < apc->num_queues; i++) { > > + txq = &apc->tx_qp[i].txq; > > + while (atomic_read(&txq->pending_sends)) { > > + skb = skb_dequeue(&txq->pending_skbs); > > + mana_unmap_skb(skb, apc); > > + dev_consume_skb_any(skb); > > + atomic_sub(1, &txq->pending_sends); > > + } > If I get this commit correctly, txq->pending_sends should be equal to > the length of txq->pending_skbs? > > If yes, can we only handle the pending_skbs? > > the above snippet can be changed to as below? So the performance is better? > " > while ((skb = skb_dequeue(&txq->pending_skbs))) { > mana_unmap_skb(skb, apc); > dev_consume_skb_any(skb); > } > atomic_set(&txq->pending_sends, 0); > " > > Zhu Yanjun Yes, we can do that, thanks for pointing. Will take care of it in next version. > > > + } > > /* 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 Wed, Jul 26, 2023 at 06:15:36AM -0700, Souradeep Chakrabarti wrote: > When unloading the MANA driver, mana_dealloc_queues() waits for the MANA > hardware to complete any inflight packets and set the pending send count > to zero. But if the hardware has failed, mana_dealloc_queues() > could wait forever. > > Fix this by adding a timeout to the wait. Set the timeout to 120 seconds, > which is a somewhat arbitrary value that is more than long enough for > functional hardware to complete any sends. > > Cc: stable@vger.kernel.org > Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)") > > Signed-off-by: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com> Hi Souradeep. The subject line of this patch seems to be cut off half way. Thanks, Wei.
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c index a499e460594b..ea039e2d4c4b 100644 --- a/drivers/net/ethernet/microsoft/mana/mana_en.c +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c @@ -8,6 +8,7 @@ #include <linux/ethtool.h> #include <linux/filter.h> #include <linux/mm.h> +#include <linux/pci.h> #include <net/checksum.h> #include <net/ip6_checksum.h> @@ -2345,9 +2346,12 @@ int mana_attach(struct net_device *ndev) static int mana_dealloc_queues(struct net_device *ndev) { struct mana_port_context *apc = netdev_priv(ndev); + unsigned long timeout = jiffies + 120 * HZ; struct gdma_dev *gd = apc->ac->gdma_dev; struct mana_txq *txq; + struct sk_buff *skb; int i, err; + u32 tsleep; if (apc->port_is_up) return -EINVAL; @@ -2363,15 +2367,41 @@ static int mana_dealloc_queues(struct net_device *ndev) * to false, but it doesn't matter since mana_start_xmit() drops any * new packets due to apc->port_is_up being false. * - * Drain all the in-flight TX packets + * Drain all the in-flight TX packets. + * A timeout of 120 seconds for all the queues is used. + * This will break the while loop when h/w is not responding. + * This value of 120 has been decided here considering max + * number of queues. */ + for (i = 0; i < apc->num_queues; i++) { txq = &apc->tx_qp[i].txq; - - while (atomic_read(&txq->pending_sends) > 0) - usleep_range(1000, 2000); + tsleep = 1000; + while (atomic_read(&txq->pending_sends) > 0 && + time_before(jiffies, timeout)) { + usleep_range(tsleep, tsleep + 1000); + tsleep <<= 1; + } + if (atomic_read(&txq->pending_sends)) { + err = pcie_flr(to_pci_dev(gd->gdma_context->dev)); + if (err) { + netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n", + err, atomic_read(&txq->pending_sends), + txq->gdma_txq_id); + } + break; + } } + for (i = 0; i < apc->num_queues; i++) { + txq = &apc->tx_qp[i].txq; + while (atomic_read(&txq->pending_sends)) { + skb = skb_dequeue(&txq->pending_skbs); + mana_unmap_skb(skb, apc); + dev_consume_skb_any(skb); + 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. */
When unloading the MANA driver, mana_dealloc_queues() waits for the MANA hardware to complete any inflight packets and set the pending send count to zero. But if the hardware has failed, mana_dealloc_queues() could wait forever. Fix this by adding a timeout to the wait. Set the timeout to 120 seconds, which is a somewhat arbitrary value that is more than long enough for functional hardware to complete any sends. Cc: stable@vger.kernel.org Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)") Signed-off-by: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com> --- V5 -> V6: * Added pcie_flr to reset the pci after timeout. * Fixed the position of changelog. * Removed unused variable like cq. V4 -> V5: * Added fixes tag * Changed the usleep_range from static to incremental value. * Initialized timeout in the begining. V3 -> V4: * Removed the unnecessary braces from mana_dealloc_queues(). V2 -> V3: * Removed the unnecessary braces from mana_dealloc_queues(). V1 -> V2: * Added net branch * Removed the typecasting to (struct mana_context*) of void pointer * Repositioned timeout variable in mana_dealloc_queues() * Repositioned vf_unload_timeout in mana_context struct, to utilise the 6 bytes hole --- drivers/net/ethernet/microsoft/mana/mana_en.c | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-)