[PATCH 2.6.26-4.14] IB/ipoib: Arm "send_cq" to process completions in due time

[PATCH 2.6.26-4.14] IB/ipoib: Arm "send_cq" to process completions in due time

[Gerd Rausch]

This issue appears to only exist in Linux versions
2.6.26 through 4.14 inclusively:

With the introduction of commit
f56bcd8013566 ("IPoIB: Use separate CQ for UD send completions")

work completions are only processed once there are
more than 17 outstanding TX work requests.

Unfortunately, that also delays the processing of the
completion handler and holds on to references
held by the "skb" since "dev_kfree_skb_any"
won't be called for a very long time.

E.g. we've observed "nf_conntrack_cleanup_net_list" spin
     around for hours until "net->ct.count" goes down to zero
     on a sufficiently idle interface.

This fix arms the TX CQ after those "poll_tx" loops,
in order for "ipoib_send_comp_handler" to do its thing:

While it's obvious that processing completions one-by-one
is more costly than doing so in bulk,
holding on to "skb" resources for a potentially unlimited
amount of time appears to be a less favorable trade-off.

This issue appears to no longer exist in Linux-4.15
and younger, because the following commit does
call "ib_req_notify_cq" on "send_cq":
8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")

Fixes: f56bcd8013566 ("IPoIB: Use separate CQ for UD send completions")

Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
---
 drivers/infiniband/ulp/ipoib/ipoib_ib.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
index 18f732aa15101..b26b31b9e455e 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
@@ -491,8 +491,13 @@ static void drain_tx_cq(struct net_device *dev)
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 
 	netif_tx_lock(dev);
-	while (poll_tx(priv))
-		; /* nothing */
+
+	do {
+		while (poll_tx(priv))
+			; /* nothing */
+	} while (ib_req_notify_cq(priv->send_cq,
+				  IB_CQ_NEXT_COMP |
+				  IB_CQ_REPORT_MISSED_EVENTS) > 0);
 
 	if (netif_queue_stopped(dev))
 		mod_timer(&priv->poll_timer, jiffies + 1);
@@ -628,9 +633,14 @@ void ipoib_send(struct net_device *dev, struct sk_buff *skb,
 		++priv->tx_head;
 	}
 
-	if (unlikely(priv->tx_outstanding > MAX_SEND_CQE))
-		while (poll_tx(priv))
-			; /* nothing */
+	if (unlikely(priv->tx_outstanding > MAX_SEND_CQE)) {
+		do {
+			while (poll_tx(priv))
+				; /* nothing */
+		} while (ib_req_notify_cq(priv->send_cq,
+					  IB_CQ_NEXT_COMP |
+					  IB_CQ_REPORT_MISSED_EVENTS) > 0);
+	}
 }
 
 static void __ipoib_reap_ah(struct net_device *dev)
-- 
2.24.1

Re: [PATCH 2.6.26-4.14] IB/ipoib: Arm "send_cq" to process completions in due time

[Jason Gunthorpe]

On Fri, Jun 12, 2020 at 12:41:16PM -0700, Gerd Rausch wrote:
> This issue appears to only exist in Linux versions
> 2.6.26 through 4.14 inclusively:
> 
> With the introduction of commit
> f56bcd8013566 ("IPoIB: Use separate CQ for UD send completions")
> 
> work completions are only processed once there are
> more than 17 outstanding TX work requests.
> 
> Unfortunately, that also delays the processing of the
> completion handler and holds on to references
> held by the "skb" since "dev_kfree_skb_any"
> won't be called for a very long time.
> 
> E.g. we've observed "nf_conntrack_cleanup_net_list" spin
>      around for hours until "net->ct.count" goes down to zero
>      on a sufficiently idle interface.
> 
> This fix arms the TX CQ after those "poll_tx" loops,
> in order for "ipoib_send_comp_handler" to do its thing:
> 
> While it's obvious that processing completions one-by-one
> is more costly than doing so in bulk,
> holding on to "skb" resources for a potentially unlimited
> amount of time appears to be a less favorable trade-off.
> 
> This issue appears to no longer exist in Linux-4.15
> and younger, because the following commit does
> call "ib_req_notify_cq" on "send_cq":
> 8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")

I'm not really clear what you want to happen to this patch - are you
proposing a stable patch that is not just a backport? Why can't you
backport the fix above instead?

You'll need to follow everything in

Documentation/process/stable-kernel-rules.rst

Or the stable maintainers won't even look at this.

Jasom

Re: [PATCH 2.6.26-4.14] IB/ipoib: Arm "send_cq" to process completions in due time

[Gerd Rausch]

Hi Jason,

On 12/06/2020 12.55, Jason Gunthorpe wrote:
> On Fri, Jun 12, 2020 at 12:41:16PM -0700, Gerd Rausch wrote:
>> This issue appears to no longer exist in Linux-4.15
>> and younger, because the following commit does
>> call "ib_req_notify_cq" on "send_cq":
>> 8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")
> 
> I'm not really clear what you want to happen to this patch - are you
> proposing a stable patch that is not just a backport? Why can't you
> backport the fix above instead?

I considered backporting commit 8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")
with all the dependencies it may have a considerably higher risk
than just arming the TX CQ.

> 
> You'll need to follow everything in
> 
> Documentation/process/stable-kernel-rules.rst
> 
> Or the stable maintainers won't even look at this.
> 

Thanks,

  Gerd

Re: [PATCH 2.6.26-4.14] IB/ipoib: Arm "send_cq" to process completions in due time

[Greg Kroah-Hartman]

On Fri, Jun 12, 2020 at 01:44:55PM -0700, Gerd Rausch wrote:
> Hi Jason,
> 
> On 12/06/2020 12.55, Jason Gunthorpe wrote:
> > On Fri, Jun 12, 2020 at 12:41:16PM -0700, Gerd Rausch wrote:
> >> This issue appears to no longer exist in Linux-4.15
> >> and younger, because the following commit does
> >> call "ib_req_notify_cq" on "send_cq":
> >> 8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")
> > 
> > I'm not really clear what you want to happen to this patch - are you
> > proposing a stable patch that is not just a backport? Why can't you
> > backport the fix above instead?
> 
> I considered backporting commit 8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")
> with all the dependencies it may have a considerably higher risk
> than just arming the TX CQ.

90% of the time when we apply a patch that does NOT match the upstream
tree, it has a bug in it and needs to have another fix or something
else.

So please, if at all possible, stick to the upstream tree, so
backporting the current patches are the best thing to do.

thanks,

greg k-h

Re: [PATCH 2.6.26-4.14] IB/ipoib: Arm "send_cq" to process completions in due time

[Gerd Rausch]

Hi,

On 16/06/2020 05.08, Greg Kroah-Hartman wrote:
>> I considered backporting commit 8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")
>> with all the dependencies it may have a considerably higher risk
>> than just arming the TX CQ.
> 
> 90% of the time when we apply a patch that does NOT match the upstream
> tree, it has a bug in it and needs to have another fix or something
> else.
> 
> So please, if at all possible, stick to the upstream tree, so
> backporting the current patches are the best thing to do.
> 

Jason,

With Mellanox writing and fixing the vast majority of the code found
in IB/IPoIB, do you or one of your colleagues want to look into this?

It would be considerably less error-prone if the authors of that code
did that more risky work of backporting.

AFAIK, Mellanox also has the regression tests to ensure that everything
still works after this re-write as it did before.

Thanks,

 Gerd

Re: [PATCH 2.6.26-4.14] IB/ipoib: Arm "send_cq" to process completions in due time

[Leon Romanovsky]

On Tue, Jun 16, 2020 at 09:35:38AM -0700, Gerd Rausch wrote:
> Hi,
>
> On 16/06/2020 05.08, Greg Kroah-Hartman wrote:
> >> I considered backporting commit 8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")
> >> with all the dependencies it may have a considerably higher risk
> >> than just arming the TX CQ.
> >
> > 90% of the time when we apply a patch that does NOT match the upstream
> > tree, it has a bug in it and needs to have another fix or something
> > else.
> >
> > So please, if at all possible, stick to the upstream tree, so
> > backporting the current patches are the best thing to do.
> >
>
> Jason,
>
> With Mellanox writing and fixing the vast majority of the code found
> in IB/IPoIB, do you or one of your colleagues want to look into this?
>
> It would be considerably less error-prone if the authors of that code
> did that more risky work of backporting.
>
> AFAIK, Mellanox also has the regression tests to ensure that everything
> still works after this re-write as it did before.

Please approach your Mellanox FAE representatives, they will know how to
handle it internally.

Thanks

>
> Thanks,
>
>  Gerd
>

Re: [PATCH 2.6.26-4.14] IB/ipoib: Arm "send_cq" to process completions in due time

[Greg Kroah-Hartman]

On Wed, Jun 17, 2020 at 08:03:41AM +0300, Leon Romanovsky wrote:
> On Tue, Jun 16, 2020 at 09:35:38AM -0700, Gerd Rausch wrote:
> > Hi,
> >
> > On 16/06/2020 05.08, Greg Kroah-Hartman wrote:
> > >> I considered backporting commit 8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")
> > >> with all the dependencies it may have a considerably higher risk
> > >> than just arming the TX CQ.
> > >
> > > 90% of the time when we apply a patch that does NOT match the upstream
> > > tree, it has a bug in it and needs to have another fix or something
> > > else.
> > >
> > > So please, if at all possible, stick to the upstream tree, so
> > > backporting the current patches are the best thing to do.
> > >
> >
> > Jason,
> >
> > With Mellanox writing and fixing the vast majority of the code found
> > in IB/IPoIB, do you or one of your colleagues want to look into this?
> >
> > It would be considerably less error-prone if the authors of that code
> > did that more risky work of backporting.
> >
> > AFAIK, Mellanox also has the regression tests to ensure that everything
> > still works after this re-write as it did before.
> 
> Please approach your Mellanox FAE representatives, they will know how to
> handle it internally.

Ah, so you all don't care about any IB fixes for 4.14 and older kernels
anymore?  If so, great, please let us know so we will not do any
backporting anymore, that will save us time!

thanks,

greg k-h

Re: [PATCH 2.6.26-4.14] IB/ipoib: Arm "send_cq" to process completions in due time

[Leon Romanovsky]

On Mon, Jul 13, 2020 at 04:53:44PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 17, 2020 at 08:03:41AM +0300, Leon Romanovsky wrote:
> > On Tue, Jun 16, 2020 at 09:35:38AM -0700, Gerd Rausch wrote:
> > > Hi,
> > >
> > > On 16/06/2020 05.08, Greg Kroah-Hartman wrote:
> > > >> I considered backporting commit 8966e28d2e40c ("IB/ipoib: Use NAPI in UD/TX flows")
> > > >> with all the dependencies it may have a considerably higher risk
> > > >> than just arming the TX CQ.
> > > >
> > > > 90% of the time when we apply a patch that does NOT match the upstream
> > > > tree, it has a bug in it and needs to have another fix or something
> > > > else.
> > > >
> > > > So please, if at all possible, stick to the upstream tree, so
> > > > backporting the current patches are the best thing to do.
> > > >
> > >
> > > Jason,
> > >
> > > With Mellanox writing and fixing the vast majority of the code found
> > > in IB/IPoIB, do you or one of your colleagues want to look into this?
> > >
> > > It would be considerably less error-prone if the authors of that code
> > > did that more risky work of backporting.
> > >
> > > AFAIK, Mellanox also has the regression tests to ensure that everything
> > > still works after this re-write as it did before.
> >
> > Please approach your Mellanox FAE representatives, they will know how to
> > handle it internally.
>
> Ah, so you all don't care about any IB fixes for 4.14 and older kernels
> anymore?  If so, great, please let us know so we will not do any
> backporting anymore, that will save us time!

Greg,

This is not what I said. As a Mellanox employee, I can't commit for any
internal resources, the FAE path is a standard way for our customers
to get proper attention.

Thanks

>
> thanks,
>
> greg k-h