[PATCH net-next] mlx4: do not fire tasklet unless necessary

From: Eric Dumazet <edumazet@google.com>

All rx and rx netdev interrupts are handled by respectively
by mlx4_en_rx_irq() and mlx4_en_tx_irq() which simply schedule a NAPI.

But mlx4_eq_int() also fires a tasklet to service all items that were
queued via mlx4_add_cq_to_tasklet(), but this handler was not called
unless user cqe was handled.

This is very confusing, as "mpstat -I SCPU ..." show huge number of
tasklet invocations.

This patch saves this overhead, by carefully firing the tasklet directly
from mlx4_add_cq_to_tasklet(), removing four atomic operations per IRQ.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tariq Toukan <redacted>
Cc: Saeed Mahameed <redacted>
---
 drivers/net/ethernet/mellanox/mlx4/cq.c |    6 +++++-
 drivers/net/ethernet/mellanox/mlx4/eq.c |    9 +--------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c
index 6b8635378f1fcb2aae4e8ac390bcd09d552c2256..fa6d2354a0e910ee160863e3cbe21a512d77bf03 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cq.c

@@ -81,8 +81,9 @@ void mlx4_cq_tasklet_cb(unsigned long data)
 
 static void mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
 {
-	unsigned long flags;
 	struct mlx4_eq_tasklet *tasklet_ctx = cq->tasklet_ctx.priv;
+	unsigned long flags;
+	bool kick;
 
 	spin_lock_irqsave(&tasklet_ctx->lock, flags);
 	/* When migrating CQs between EQs will be implemented, please note

@@ -92,7 +93,10 @@ static void mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
 	 */
 	if (list_empty_careful(&cq->tasklet_ctx.list)) {
 		atomic_inc(&cq->refcount);
+		kick = list_empty(&tasklet_ctx->list);
 		list_add_tail(&cq->tasklet_ctx.list, &tasklet_ctx->list);
+		if (kick)
+			tasklet_schedule(&tasklet_ctx->task);
 	}
 	spin_unlock_irqrestore(&tasklet_ctx->lock, flags);
 }

diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c b/drivers/net/ethernet/mellanox/mlx4/eq.c
index 0509996957d9664b612358dd805359f4bc67b8dc..39232b6a974f4b4b961d3b0b8634f04e6b9d0caa 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c

@@ -494,7 +494,7 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
 {
 	struct mlx4_priv *priv = mlx4_priv(dev);
 	struct mlx4_eqe *eqe;
-	int cqn = -1;
+	int cqn;
 	int eqes_found = 0;
 	int set_ci = 0;
 	int port;

@@ -840,13 +840,6 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
 
 	eq_set_ci(eq, 1);
 
-	/* cqn is 24bit wide but is initialized such that its higher bits
-	 * are ones too. Thus, if we got any event, cqn's high bits should be off
-	 * and we need to schedule the tasklet.
-	 */
-	if (!(cqn & ~0xffffff))
-		tasklet_schedule(&eq->tasklet_ctx.task);
-
 	return eqes_found;
 }
 

From: Eric Dumazet <redacted>
Date: Fri, 10 Feb 2017 04:27:58 -0800

From: Eric Dumazet <edumazet@google.com>

All rx and rx netdev interrupts are handled by respectively
by mlx4_en_rx_irq() and mlx4_en_tx_irq() which simply schedule a NAPI.

But mlx4_eq_int() also fires a tasklet to service all items that were
queued via mlx4_add_cq_to_tasklet(), but this handler was not called
unless user cqe was handled.

This is very confusing, as "mpstat -I SCPU ..." show huge number of
tasklet invocations.

This patch saves this overhead, by carefully firing the tasklet directly
from mlx4_add_cq_to_tasklet(), removing four atomic operations per IRQ.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tariq Toukan <redacted>
Cc: Saeed Mahameed <redacted>

Tariq/Saeed, please review.

On Fri, Feb 10, 2017 at 2:27 PM, Eric Dumazet [off-list ref] wrote:

So first one in would fire the tasklet, but wouldn't this cause CQE
processing loss
in the same mlx4_eq_int loop if the tasklet was fast enough to
schedule and while other CQEs are going to add themselves to the
tasklet_ctx->list ?

Anyway i tried to find race scenarios that could cause such thing but
synchronization looks good.

what if we simply change this condition to:
if (!list_empty_careful(eq->tasklet_ctx.list))

Wouldn't this be sort of equivalent to what you did ? and this way we
would simply fire the tasklet only when needed and not on every
handled CQE.

-               tasklet_schedule(&eq->tasklet_ctx.task);
-
        return eqes_found;
 }

On Wed, 2017-02-15 at 13:10 +0200, Saeed Mahameed wrote:

On Fri, Feb 10, 2017 at 2:27 PM, Eric Dumazet [off-list ref] wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

All rx and rx netdev interrupts are handled by respectively
by mlx4_en_rx_irq() and mlx4_en_tx_irq() which simply schedule a NAPI.

But mlx4_eq_int() also fires a tasklet to service all items that were
queued via mlx4_add_cq_to_tasklet(), but this handler was not called
unless user cqe was handled.

This is very confusing, as "mpstat -I SCPU ..." show huge number of
tasklet invocations.

This patch saves this overhead, by carefully firing the tasklet directly
from mlx4_add_cq_to_tasklet(), removing four atomic operations per IRQ.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tariq Toukan <redacted>
Cc: Saeed Mahameed <redacted>
---
 drivers/net/ethernet/mellanox/mlx4/cq.c |    6 +++++-
 drivers/net/ethernet/mellanox/mlx4/eq.c |    9 +--------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c
index 6b8635378f1fcb2aae4e8ac390bcd09d552c2256..fa6d2354a0e910ee160863e3cbe21a512d77bf03 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cq.c

@@ -81,8 +81,9 @@ void mlx4_cq_tasklet_cb(unsigned long data)

 static void mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
 {
-       unsigned long flags;
        struct mlx4_eq_tasklet *tasklet_ctx = cq->tasklet_ctx.priv;
+       unsigned long flags;
+       bool kick;

        spin_lock_irqsave(&tasklet_ctx->lock, flags);
        /* When migrating CQs between EQs will be implemented, please note

@@ -92,7 +93,10 @@ static void mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
         */
        if (list_empty_careful(&cq->tasklet_ctx.list)) {
                atomic_inc(&cq->refcount);
+               kick = list_empty(&tasklet_ctx->list);

So first one in would fire the tasklet, but wouldn't this cause CQE
processing loss
in the same mlx4_eq_int loop if the tasklet was fast enough to
schedule and while other CQEs are going to add themselves to the
tasklet_ctx->list ?

mlx4_eq_int() is a hard irq handler.

How a tasklet could run in the middle of it ?

A tasklet is a softirq handler.

softirq must wait that the current hard irq handler is done.

Anyway i tried to find race scenarios that could cause such thing but
synchronization looks good.

quoted

                list_add_tail(&cq->tasklet_ctx.list, &tasklet_ctx->list);
+               if (kick)
+                       tasklet_schedule(&tasklet_ctx->task);
        }
        spin_unlock_irqrestore(&tasklet_ctx->lock, flags);
 }

diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c b/drivers/net/ethernet/mellanox/mlx4/eq.c
index 0509996957d9664b612358dd805359f4bc67b8dc..39232b6a974f4b4b961d3b0b8634f04e6b9d0caa 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c

@@ -494,7 +494,7 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
 {
        struct mlx4_priv *priv = mlx4_priv(dev);
        struct mlx4_eqe *eqe;
-       int cqn = -1;
+       int cqn;
        int eqes_found = 0;
        int set_ci = 0;
        int port;

@@ -840,13 +840,6 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)

        eq_set_ci(eq, 1);

-       /* cqn is 24bit wide but is initialized such that its higher bits
-        * are ones too. Thus, if we got any event, cqn's high bits should be off
-        * and we need to schedule the tasklet.
-        */
-       if (!(cqn & ~0xffffff))

what if we simply change this condition to:
if (!list_empty_careful(eq->tasklet_ctx.list))

Wouldn't this be sort of equivalent to what you did ? and this way we
would simply fire the tasklet only when needed and not on every
handled CQE.

Still this test would be done one million time per second on my hosts.

What is the point exactly ?

Thanks.

On Wed, Feb 15, 2017 at 3:29 PM, Eric Dumazet [off-list ref] wrote:

On Wed, 2017-02-15 at 13:10 +0200, Saeed Mahameed wrote:

quoted

On Fri, Feb 10, 2017 at 2:27 PM, Eric Dumazet [off-list ref] wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

All rx and rx netdev interrupts are handled by respectively
by mlx4_en_rx_irq() and mlx4_en_tx_irq() which simply schedule a NAPI.

But mlx4_eq_int() also fires a tasklet to service all items that were
queued via mlx4_add_cq_to_tasklet(), but this handler was not called
unless user cqe was handled.

This is very confusing, as "mpstat -I SCPU ..." show huge number of
tasklet invocations.

This patch saves this overhead, by carefully firing the tasklet directly
from mlx4_add_cq_to_tasklet(), removing four atomic operations per IRQ.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tariq Toukan <redacted>
Cc: Saeed Mahameed <redacted>
---
 drivers/net/ethernet/mellanox/mlx4/cq.c |    6 +++++-
 drivers/net/ethernet/mellanox/mlx4/eq.c |    9 +--------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c
index 6b8635378f1fcb2aae4e8ac390bcd09d552c2256..fa6d2354a0e910ee160863e3cbe21a512d77bf03 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cq.c

@@ -81,8 +81,9 @@ void mlx4_cq_tasklet_cb(unsigned long data)

 static void mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
 {
-       unsigned long flags;
        struct mlx4_eq_tasklet *tasklet_ctx = cq->tasklet_ctx.priv;
+       unsigned long flags;
+       bool kick;

        spin_lock_irqsave(&tasklet_ctx->lock, flags);
        /* When migrating CQs between EQs will be implemented, please note

@@ -92,7 +93,10 @@ static void mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
         */
        if (list_empty_careful(&cq->tasklet_ctx.list)) {
                atomic_inc(&cq->refcount);
+               kick = list_empty(&tasklet_ctx->list);

So first one in would fire the tasklet, but wouldn't this cause CQE
processing loss
in the same mlx4_eq_int loop if the tasklet was fast enough to
schedule and while other CQEs are going to add themselves to the
tasklet_ctx->list ?

mlx4_eq_int() is a hard irq handler.

How a tasklet could run in the middle of it ?

can the tasklet run on a different core ?

A tasklet is a softirq handler.

softirq must wait that the current hard irq handler is done.

quoted

Anyway i tried to find race scenarios that could cause such thing but
synchronization looks good.

quoted

                list_add_tail(&cq->tasklet_ctx.list, &tasklet_ctx->list);
+               if (kick)
+                       tasklet_schedule(&tasklet_ctx->task);
        }
        spin_unlock_irqrestore(&tasklet_ctx->lock, flags);
 }

diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c b/drivers/net/ethernet/mellanox/mlx4/eq.c
index 0509996957d9664b612358dd805359f4bc67b8dc..39232b6a974f4b4b961d3b0b8634f04e6b9d0caa 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c

@@ -494,7 +494,7 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)
 {
        struct mlx4_priv *priv = mlx4_priv(dev);
        struct mlx4_eqe *eqe;
-       int cqn = -1;
+       int cqn;
        int eqes_found = 0;
        int set_ci = 0;
        int port;

@@ -840,13 +840,6 @@ static int mlx4_eq_int(struct mlx4_dev *dev, struct mlx4_eq *eq)

        eq_set_ci(eq, 1);

-       /* cqn is 24bit wide but is initialized such that its higher bits
-        * are ones too. Thus, if we got any event, cqn's high bits should be off
-        * and we need to schedule the tasklet.
-        */
-       if (!(cqn & ~0xffffff))

what if we simply change this condition to:
if (!list_empty_careful(eq->tasklet_ctx.list))

Wouldn't this be sort of equivalent to what you did ? and this way we
would simply fire the tasklet only when needed and not on every
handled CQE.

Still this test would be done one million time per second on my hosts.

What is the point exactly ?

the point is that if the EQ is full of CQEs from different CQs you would
do the "  kick = list_empty(&tasklet_ctx->list);" test per empty CQ
list rather than once at the end.

in mlx4_en case, you have only two CQs on each EQ but in RoCE/IB you
can have as many CQs as you want.

Thanks.

On Wed, 2017-02-15 at 05:29 -0800, Eric Dumazet wrote:

mlx4_eq_int() is a hard irq handler.

How a tasklet could run in the middle of it ?

A tasklet is a softirq handler.

Speaking of mlx4_eq_int() , 50% of cycles are spent on mb() (mfence)
in eq_set_ci()

I wonder why this very expensive mb() is required, right before exiting
the interrupt handler.
 

On Wed, 2017-02-15 at 15:59 +0200, Saeed Mahameed wrote:

can the tasklet run on a different core ?

No. tasklets are scheduled on local cpu, like softirqs in general.

They are not migrated, unless cpu is removed (hotplug)

the point is that if the EQ is full of CQEs from different CQs you would
do the "  kick = list_empty(&tasklet_ctx->list);" test per empty CQ
list rather than once at the end.

in mlx4_en case, you have only two CQs on each EQ but in RoCE/IB you
can have as many CQs as you want.

list_empty() before one list_add_tail() is a single instruction.

By doing this right before manipulating the list, it comes with a zero
cache line penalty.

While if you do it at the wrong place, it incurs one extra cache line
miss.

This extra tasklet invocations can cost 3 % of cpu cycles.

On 15/02/2017 13:10, Saeed Mahameed wrote:

On Fri, Feb 10, 2017 at 2:27 PM, Eric Dumazet [off-list ref] wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

All rx and rx netdev interrupts are handled by respectively
by mlx4_en_rx_irq() and mlx4_en_tx_irq() which simply schedule a NAPI.

But mlx4_eq_int() also fires a tasklet to service all items that were
queued via mlx4_add_cq_to_tasklet(), but this handler was not called
unless user cqe was handled.

This is very confusing, as "mpstat -I SCPU ..." show huge number of
tasklet invocations.

This patch saves this overhead, by carefully firing the tasklet directly
from mlx4_add_cq_to_tasklet(), removing four atomic operations per IRQ.

So, in case of RDMA CQs, we add some per-CQE overhead of comparing the 
list pointers and condition upon that. Maybe we could add an 
invoke_tasklet boolean field on mlx4_cq and return its value from 
mlx4_cq_completion.
That's way we could do invoke_tasklet |= mlx4_cq_completion(....);

Outside the while loop we could just
if (invoke_tasklet)
     tasklet_schedule

Anyway, I guess that even with per-CQE overhead, the performance impact 
here is pretty negligible - so I guess that's fine too :)

On Wed, 2017-02-15 at 16:52 +0200, Matan Barak (External) wrote:

So, in case of RDMA CQs, we add some per-CQE overhead of comparing the 
list pointers and condition upon that. Maybe we could add an 
invoke_tasklet boolean field on mlx4_cq and return its value from 
mlx4_cq_completion.
That's way we could do invoke_tasklet |= mlx4_cq_completion(....);

Outside the while loop we could just
if (invoke_tasklet)
     tasklet_schedule

Anyway, I guess that even with per-CQE overhead, the performance impact 
here is pretty negligible - so I guess that's fine too :)

Real question or suggestion would be to use/fire a tasklet only under
stress.

Firing a tasklet adds a lot of latencies for user-space CQ completion,
since softirqs might have to be handled by a kernel thread (ksoftirqd)

I would be surprised if no customer was hit by your commit,
( net/mlx4_core: Use tasklet for user-space CQ completion events )
especially when using specific (RT) scheduler classes.

On Wed, Feb 15, 2017 at 5:26 PM, Eric Dumazet [off-list ref] wrote:

On Wed, 2017-02-15 at 16:52 +0200, Matan Barak (External) wrote:

quoted

So, in case of RDMA CQs, we add some per-CQE overhead of comparing the
list pointers and condition upon that. Maybe we could add an
invoke_tasklet boolean field on mlx4_cq and return its value from
mlx4_cq_completion.
That's way we could do invoke_tasklet |= mlx4_cq_completion(....);

Outside the while loop we could just
if (invoke_tasklet)
     tasklet_schedule

Anyway, I guess that even with per-CQE overhead, the performance impact
here is pretty negligible - so I guess that's fine too :)

Real question or suggestion would be to use/fire a tasklet only under
stress.

Firing a tasklet adds a lot of latencies for user-space CQ completion,
since softirqs might have to be handled by a kernel thread (ksoftirqd)

At least for mlx4_en driver we don't need this tasklet and it is only
adding this overhead. (we have napi)

we must consider removing it for mlx4_en cqs and move the tasklet
handling to mlx4_ib.

I will ack the patch.

On Fri, Feb 10, 2017 at 2:27 PM, Eric Dumazet [off-list ref] wrote:

From: Eric Dumazet <edumazet@google.com>

All rx and rx netdev interrupts are handled by respectively
by mlx4_en_rx_irq() and mlx4_en_tx_irq() which simply schedule a NAPI.

But mlx4_eq_int() also fires a tasklet to service all items that were
queued via mlx4_add_cq_to_tasklet(), but this handler was not called
unless user cqe was handled.

This is very confusing, as "mpstat -I SCPU ..." show huge number of
tasklet invocations.

This patch saves this overhead, by carefully firing the tasklet directly
from mlx4_add_cq_to_tasklet(), removing four atomic operations per IRQ.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tariq Toukan <redacted>
Cc: Saeed Mahameed <redacted>

Acked-by: Saeed Mahameed <redacted>

On Wed, Feb 15, 2017 at 4:04 PM, Eric Dumazet [off-list ref] wrote:

On Wed, 2017-02-15 at 05:29 -0800, Eric Dumazet wrote:

quoted

mlx4_eq_int() is a hard irq handler.

How a tasklet could run in the middle of it ?

A tasklet is a softirq handler.

Speaking of mlx4_eq_int() , 50% of cycles are spent on mb() (mfence)
in eq_set_ci()

I wonder why you have so many interrupts ? don't you have some kind of
interrupt moderation ?
what test are you running that got your CPU so busy.

I wonder why this very expensive mb() is required, right before exiting
the interrupt handler.

to make sure the HW knows we handled Completions up to (ci) consumer
index. so it will generate next irq.

On Thu, 2017-02-16 at 14:38 +0200, Saeed Mahameed wrote:

Acked-by: Saeed Mahameed <redacted>

Thanks for reviewing this Saeed !

On Thu, 2017-02-16 at 14:44 +0200, Saeed Mahameed wrote:

On Wed, Feb 15, 2017 at 4:04 PM, Eric Dumazet [off-list ref] wrote:

quoted

On Wed, 2017-02-15 at 05:29 -0800, Eric Dumazet wrote:

quoted

mlx4_eq_int() is a hard irq handler.

How a tasklet could run in the middle of it ?

A tasklet is a softirq handler.

Speaking of mlx4_eq_int() , 50% of cycles are spent on mb() (mfence)
in eq_set_ci()

I wonder why you have so many interrupts ? don't you have some kind of
interrupt moderation ?
what test are you running that got your CPU so busy.

Simply 8 RX queues. About 140,000 irq per second per RX queue,
for a moderate network load on 40Gbit NIC.

Interrupt moderation is a latency killer, we want our usec back.

quoted

I wonder why this very expensive mb() is required, right before exiting
the interrupt handler.

to make sure the HW knows we handled Completions up to (ci) consumer
index. so it will generate next irq.

So why a mb() is needed exactly ?

wmb() seems enough.

On Thu, Feb 16, 2017 at 5:49 PM, Eric Dumazet [off-list ref] wrote:

On Thu, 2017-02-16 at 14:44 +0200, Saeed Mahameed wrote:

quoted

On Wed, Feb 15, 2017 at 4:04 PM, Eric Dumazet [off-list ref] wrote:

quoted

On Wed, 2017-02-15 at 05:29 -0800, Eric Dumazet wrote:

quoted

mlx4_eq_int() is a hard irq handler.

How a tasklet could run in the middle of it ?

A tasklet is a softirq handler.

Speaking of mlx4_eq_int() , 50% of cycles are spent on mb() (mfence)
in eq_set_ci()

I wonder why you have so many interrupts ? don't you have some kind of
interrupt moderation ?
what test are you running that got your CPU so busy.

Simply 8 RX queues. About 140,000 irq per second per RX queue,
for a moderate network load on 40Gbit NIC.

so i guess you are not busy polling .. and adaptive moderation decided
to lower down
rx-usecs for you, and you are looking to improve latency.

Interrupt moderation is a latency killer, we want our usec back.

well, for RX we have adaptive moderation.
and for TX we have xmit more.
so interrupt moderation can be good if it is done right.

do i understand from this that you are against interrupt moderation ?

quoted

quoted

I wonder why this very expensive mb() is required, right before exiting
the interrupt handler.

to make sure the HW knows we handled Completions up to (ci) consumer
index. so it will generate next irq.

So why a mb() is needed exactly ?

wmb() seems enough.

Yes seems right, not sure why it is mb() though, i will have to check
and get back to you on this.

On Thu, 2017-02-16 at 23:17 +0200, Saeed Mahameed wrote:

so i guess you are not busy polling .. and adaptive moderation decided
to lower down
rx-usecs for you, and you are looking to improve latency.

quoted

Interrupt moderation is a latency killer, we want our usec back.

well, for RX we have adaptive moderation.
and for TX we have xmit more.
so interrupt moderation can be good if it is done right.

do i understand from this that you are against interrupt moderation ?

Absolutely against it. We prefer gro_flush_timeout.

The defaults values in mlx4 never were tuned for 40Gbit.

If you want to be able to reach 40Gbit on a single TCP flow, you want :

ethtool -C ethX rx-frames 16 rx-usecs-high 32

Or disable interrupt mitigation of course.


Interrupt moderation comes for free with NAPI really :

If under stress, number of interrupts will naturally decrease,
so what's the point with hardware _delaying_ an incoming packet
exactly ???

BTW interrupt moderation in mlx4 has at least two bugs.

I will send a patch.

quoted

quoted

quoted

I wonder why this very expensive mb() is required, right before exiting
the interrupt handler.

to make sure the HW knows we handled Completions up to (ci) consumer
index. so it will generate next irq.

So why a mb() is needed exactly ?

wmb() seems enough.

Yes seems right, not sure why it is mb() though, i will have to check
and get back to you on this.

Thanks.

From: Eric Dumazet <edumazet@google.com>

1) In the case where rate == priv->pkt_rate_low == priv->pkt_rate_high,
mlx4_en_auto_moderation() does a divide by zero.

2) We want to properly change the moderation parameters if rx_frames
was changed (like in ethtool -C eth0 rx-frames 16)
 
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |   24 ++++++++-------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 3b4961a8e8e44d6987ebd23f9239e747c7fc6cd5..ab3801d724ede4ab19958b2a1a214cfbe901c370 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c

@@ -1384,6 +1384,7 @@ static void mlx4_en_set_default_moderation(struct mlx4_en_priv *priv)
 static void mlx4_en_auto_moderation(struct mlx4_en_priv *priv)
 {
 	unsigned long period = (unsigned long) (jiffies - priv->last_moder_jiffies);
+	u32 pkt_rate_high, pkt_rate_low;
 	struct mlx4_en_cq *cq;
 	unsigned long packets;
 	unsigned long rate;

@@ -1397,37 +1398,40 @@ static void mlx4_en_auto_moderation(struct mlx4_en_priv *priv)
 	if (!priv->adaptive_rx_coal || period < priv->sample_interval * HZ)
 		return;
 
+	pkt_rate_low = READ_ONCE(priv->pkt_rate_low);
+	pkt_rate_high = READ_ONCE(priv->pkt_rate_high);
+
 	for (ring = 0; ring < priv->rx_ring_num; ring++) {
 		rx_packets = READ_ONCE(priv->rx_ring[ring]->packets);
 		rx_bytes = READ_ONCE(priv->rx_ring[ring]->bytes);
 
-		rx_pkt_diff = ((unsigned long) (rx_packets -
-				priv->last_moder_packets[ring]));
+		rx_pkt_diff = rx_packets - priv->last_moder_packets[ring];
 		packets = rx_pkt_diff;
 		rate = packets * HZ / period;
-		avg_pkt_size = packets ? ((unsigned long) (rx_bytes -
-				priv->last_moder_bytes[ring])) / packets : 0;
+		avg_pkt_size = packets ? (rx_bytes -
+				priv->last_moder_bytes[ring]) / packets : 0;
 
 		/* Apply auto-moderation only when packet rate
 		 * exceeds a rate that it matters */
 		if (rate > (MLX4_EN_RX_RATE_THRESH / priv->rx_ring_num) &&
 		    avg_pkt_size > MLX4_EN_AVG_PKT_SMALL) {
-			if (rate < priv->pkt_rate_low)
+			if (rate <= pkt_rate_low)
 				moder_time = priv->rx_usecs_low;
-			else if (rate > priv->pkt_rate_high)
+			else if (rate >= pkt_rate_high)
 				moder_time = priv->rx_usecs_high;
 			else
-				moder_time = (rate - priv->pkt_rate_low) *
+				moder_time = (rate - pkt_rate_low) *
 					(priv->rx_usecs_high - priv->rx_usecs_low) /
-					(priv->pkt_rate_high - priv->pkt_rate_low) +
+					(pkt_rate_high - pkt_rate_low) +
 					priv->rx_usecs_low;
 		} else {
 			moder_time = priv->rx_usecs_low;
 		}
 
-		if (moder_time != priv->last_moder_time[ring]) {
+		cq = priv->rx_cq[ring];
+		if (moder_time != priv->last_moder_time[ring] ||
+		    cq->moder_cnt != priv->rx_frames) {
 			priv->last_moder_time[ring] = moder_time;
-			cq = priv->rx_cq[ring];
 			cq->moder_time = moder_time;
 			cq->moder_cnt = priv->rx_frames;
 			err = mlx4_en_set_cq_moder(priv, cq);

From: Eric Dumazet <redacted>
Date: Fri, 10 Feb 2017 04:27:58 -0800

From: Eric Dumazet <edumazet@google.com>

All rx and rx netdev interrupts are handled by respectively
by mlx4_en_rx_irq() and mlx4_en_tx_irq() which simply schedule a NAPI.

But mlx4_eq_int() also fires a tasklet to service all items that were
queued via mlx4_add_cq_to_tasklet(), but this handler was not called
unless user cqe was handled.

This is very confusing, as "mpstat -I SCPU ..." show huge number of
tasklet invocations.

This patch saves this overhead, by carefully firing the tasklet directly
from mlx4_add_cq_to_tasklet(), removing four atomic operations per IRQ.

Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks.

On Thu, 2017-02-16 at 07:30 -0800, Eric Dumazet wrote:

On Thu, 2017-02-16 at 14:38 +0200, Saeed Mahameed wrote:

quoted

Acked-by: Saeed Mahameed <redacted>

Thanks for reviewing this Saeed !

Note that mlx4_add_cq_to_tasklet() is called from hard irq context, so
we could replace the spin_lock_irqsave() by spin_lock()

Not that I care, but you might be interested ;)

Thanks.

diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c
index fa6d2354a0e910ee160863e3cbe21a512d77bf03..9dcac3a367425f2804e933842698c66685baf626 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cq.c

@@ -79,13 +79,13 @@ void mlx4_cq_tasklet_cb(unsigned long data)
 		tasklet_schedule(&ctx->task);
 }
 
+/* Called from hard irq handler */
 static void mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
 {
 	struct mlx4_eq_tasklet *tasklet_ctx = cq->tasklet_ctx.priv;
-	unsigned long flags;
 	bool kick;
 
-	spin_lock_irqsave(&tasklet_ctx->lock, flags);
+	spin_lock(&tasklet_ctx->lock);
 	/* When migrating CQs between EQs will be implemented, please note
 	 * that you need to sync this point. It is possible that
 	 * while migrating a CQ, completions on the old EQs could

@@ -98,7 +98,7 @@ static void mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
 		if (kick)
 			tasklet_schedule(&tasklet_ctx->task);
 	}
-	spin_unlock_irqrestore(&tasklet_ctx->lock, flags);
+	spin_unlock(&tasklet_ctx->lock);
 }
 
 void mlx4_cq_completion(struct mlx4_dev *dev, u32 cqn)

On Fri, Feb 17, 2017 at 1:23 AM, Eric Dumazet [off-list ref] wrote:

From: Eric Dumazet <edumazet@google.com>

1) In the case where rate == priv->pkt_rate_low == priv->pkt_rate_high,
mlx4_en_auto_moderation() does a divide by zero.

2) We want to properly change the moderation parameters if rx_frames
was changed (like in ethtool -C eth0 rx-frames 16)

Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Saeed Mahameed <redacted>

On Fri, Feb 17, 2017 at 1:13 AM, Eric Dumazet [off-list ref] wrote:

On Thu, 2017-02-16 at 23:17 +0200, Saeed Mahameed wrote:

quoted

so i guess you are not busy polling .. and adaptive moderation decided
to lower down
rx-usecs for you, and you are looking to improve latency.

quoted

Interrupt moderation is a latency killer, we want our usec back.

well, for RX we have adaptive moderation.
and for TX we have xmit more.
so interrupt moderation can be good if it is done right.

do i understand from this that you are against interrupt moderation ?

Absolutely against it. We prefer gro_flush_timeout.

The defaults values in mlx4 never were tuned for 40Gbit.

If you want to be able to reach 40Gbit on a single TCP flow, you want :

ethtool -C ethX rx-frames 16 rx-usecs-high 32

Or disable interrupt mitigation of course.


Interrupt moderation comes for free with NAPI really :

If under stress, number of interrupts will naturally decrease,
so what's the point with hardware _delaying_ an incoming packet
exactly ???

Theoretically NAPI provides some kind of interrupt delaying mechanism
(only under stress), but is it as good as explicit HW interrupt
moderation ?

for example in mlx5 the adaptive mechanism is more intelligent than
the one we have in mlx4.
it always tries to improve BW/packet rate, and reduce # of interrupts
per second.
and it will park at the optimal values.

As you know the less the interrupts overhead the better the latency.

I assume there are many cases where napi will not be good enough and
will keep running at max interrupt rate.
while the above mechanism will deliberately look for and find a better
interrupt rate for at least the same packet rate or even better one.
(Napi will not do that for you).

But we will have to measure before we decide napi as is is good enough.

BTW interrupt moderation in mlx4 has at least two bugs.

I will send a patch.

Saw it, nice catch !!

On Sat, Feb 18, 2017 at 4:46 AM, Eric Dumazet [off-list ref] wrote:

On Thu, 2017-02-16 at 07:30 -0800, Eric Dumazet wrote:

quoted

On Thu, 2017-02-16 at 14:38 +0200, Saeed Mahameed wrote:

quoted

Acked-by: Saeed Mahameed <redacted>

Thanks for reviewing this Saeed !

Note that mlx4_add_cq_to_tasklet() is called from hard irq context, so
we could replace the spin_lock_irqsave() by spin_lock()

Not that I care, but you might be interested ;)

Sure we are !
we will take it from here :-).

Thank you Eric.