[PATCH net 0/2] sfc: couple of ARFS fixes

[PATCH net 0/2] sfc: couple of ARFS fixes

[Edward Cree]

Two issues introduced by my recent asynchronous filter handling changes:
1. The old filter_rfs_insert would replace a matching filter of equal
   priority; we need to pass the appropriate argument to filter_insert to
   make it do the same.
2. It's possible to cause the kernel to hammer ndo_rx_flow_steer very
   hard, so make sure we don't build up too huge a backlog of workitems.

Possibly it would be better to fix #2 on the kernel side; I think the way
 to do that would be to maintain a forward (as well as reverse) queue-to-
 cpu map and replace the set_rps_cpu() check
    if (rxq_index == skb_get_rx_queue(skb))
 with something like (pseudocode)
    if (irqaffinity of queue[skb_get_rx_queue(skb)] includes next_cpu)
 but I'm not sure whether it's right or even necessary, and in any case
 it's not a regression in 4.17 so isn't 'net' material.
(There's also the issue that we come up in the bad configuration by
 default, but that too is a problem for another time.)

Edward Cree (2):
  sfc: insert ARFS filters with replace_equal=true
  sfc: limit ARFS workitems in flight per channel

 drivers/net/ethernet/sfc/net_driver.h | 25 +++++++++++++++
 drivers/net/ethernet/sfc/rx.c         | 60 ++++++++++++++++++-----------------
 2 files changed, 56 insertions(+), 29 deletions(-)

[PATCH net 1/2] sfc: insert ARFS filters with replace_equal=true

[Edward Cree]

Necessary to allow redirecting a flow when the application moves.

Fixes: 3af0f34290f6 ("sfc: replace asynchronous filter operations")
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 95682831484e..13b0eb71dbf3 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -851,7 +851,7 @@ static void efx_filter_rfs_work(struct work_struct *data)
 	struct efx_channel *channel = efx_get_channel(efx, req->rxq_index);
 	int rc;
 
-	rc = efx->type->filter_insert(efx, &req->spec, false);
+	rc = efx->type->filter_insert(efx, &req->spec, true);
 	if (rc >= 0) {
 		/* Remember this so we can check whether to expire the filter
 		 * later.

[PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[Edward Cree]

A misconfigured system (e.g. with all interrupts affinitised to all CPUs)
 may produce a storm of ARFS steering events.  With the existing sfc ARFS
 implementation, that could create a backlog of workitems that grinds the
 system to a halt.  To prevent this, limit the number of workitems that
 may be in flight for a given SFC device to 8 (EFX_RPS_MAX_IN_FLIGHT), and
 return EBUSY from our ndo_rx_flow_steer method if the limit is reached.
Given this limit, also store the workitems in an array of slots within the
 struct efx_nic, rather than dynamically allocating for each request.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/net_driver.h | 25 +++++++++++++++
 drivers/net/ethernet/sfc/rx.c         | 58 ++++++++++++++++++-----------------
 2 files changed, 55 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 5e379a83c729..eea3808b3f25 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -733,6 +733,27 @@ struct efx_rss_context {
 	u32 rx_indir_table[128];
 };
 
+#ifdef CONFIG_RFS_ACCEL
+/**
+ * struct efx_async_filter_insertion - Request to asynchronously insert a filter
+ * @net_dev: Reference to the netdevice
+ * @spec: The filter to insert
+ * @work: Workitem for this request
+ * @rxq_index: Identifies the channel for which this request was made
+ * @flow_id: Identifies the kernel-side flow for which this request was made
+ */
+struct efx_async_filter_insertion {
+	struct net_device *net_dev;
+	struct efx_filter_spec spec;
+	struct work_struct work;
+	u16 rxq_index;
+	u32 flow_id;
+};
+
+/* Maximum number of ARFS workitems that may be in flight on an efx_nic */
+#define EFX_RPS_MAX_IN_FLIGHT	8
+#endif /* CONFIG_RFS_ACCEL */
+
 /**
  * struct efx_nic - an Efx NIC
  * @name: Device name (net device name or bus id before net device registered)
@@ -850,6 +871,8 @@ struct efx_rss_context {
  * @rps_expire_channel: Next channel to check for expiry
  * @rps_expire_index: Next index to check for expiry in
  *	@rps_expire_channel's @rps_flow_id
+ * @rps_slot_map: bitmap of in-flight entries in @rps_slot
+ * @rps_slot: array of ARFS insertion requests for efx_filter_rfs_work()
  * @active_queues: Count of RX and TX queues that haven't been flushed and drained.
  * @rxq_flush_pending: Count of number of receive queues that need to be flushed.
  *	Decremented when the efx_flush_rx_queue() is called.
@@ -1004,6 +1027,8 @@ struct efx_nic {
 	struct mutex rps_mutex;
 	unsigned int rps_expire_channel;
 	unsigned int rps_expire_index;
+	unsigned long rps_slot_map;
+	struct efx_async_filter_insertion rps_slot[EFX_RPS_MAX_IN_FLIGHT];
 #endif
 
 	atomic_t active_queues;
diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 13b0eb71dbf3..9c593c661cbf 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -827,28 +827,13 @@ MODULE_PARM_DESC(rx_refill_threshold,
 
 #ifdef CONFIG_RFS_ACCEL
 
-/**
- * struct efx_async_filter_insertion - Request to asynchronously insert a filter
- * @net_dev: Reference to the netdevice
- * @spec: The filter to insert
- * @work: Workitem for this request
- * @rxq_index: Identifies the channel for which this request was made
- * @flow_id: Identifies the kernel-side flow for which this request was made
- */
-struct efx_async_filter_insertion {
-	struct net_device *net_dev;
-	struct efx_filter_spec spec;
-	struct work_struct work;
-	u16 rxq_index;
-	u32 flow_id;
-};
-
 static void efx_filter_rfs_work(struct work_struct *data)
 {
 	struct efx_async_filter_insertion *req = container_of(data, struct efx_async_filter_insertion,
 							      work);
 	struct efx_nic *efx = netdev_priv(req->net_dev);
 	struct efx_channel *channel = efx_get_channel(efx, req->rxq_index);
+	int slot_idx = req - efx->rps_slot;
 	int rc;
 
 	rc = efx->type->filter_insert(efx, &req->spec, true);
@@ -878,8 +863,8 @@ static void efx_filter_rfs_work(struct work_struct *data)
 	}
 
 	/* Release references */
+	clear_bit(slot_idx, &efx->rps_slot_map);
 	dev_put(req->net_dev);
-	kfree(req);
 }
 
 int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
@@ -888,22 +873,36 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 	struct efx_nic *efx = netdev_priv(net_dev);
 	struct efx_async_filter_insertion *req;
 	struct flow_keys fk;
+	int slot_idx;
+	int rc;
 
-	if (flow_id == RPS_FLOW_ID_INVALID)
-		return -EINVAL;
+	/* find a free slot */
+	for (slot_idx = 0; slot_idx < EFX_RPS_MAX_IN_FLIGHT; slot_idx++)
+		if (!test_and_set_bit(slot_idx, &efx->rps_slot_map))
+			break;
+	if (slot_idx >= EFX_RPS_MAX_IN_FLIGHT)
+		return -EBUSY;
 
-	if (!skb_flow_dissect_flow_keys(skb, &fk, 0))
-		return -EPROTONOSUPPORT;
+	if (flow_id == RPS_FLOW_ID_INVALID) {
+		rc = -EINVAL;
+		goto out_clear;
+	}
 
-	if (fk.basic.n_proto != htons(ETH_P_IP) && fk.basic.n_proto != htons(ETH_P_IPV6))
-		return -EPROTONOSUPPORT;
-	if (fk.control.flags & FLOW_DIS_IS_FRAGMENT)
-		return -EPROTONOSUPPORT;
+	if (!skb_flow_dissect_flow_keys(skb, &fk, 0)) {
+		rc = -EPROTONOSUPPORT;
+		goto out_clear;
+	}
 
-	req = kmalloc(sizeof(*req), GFP_ATOMIC);
-	if (!req)
-		return -ENOMEM;
+	if (fk.basic.n_proto != htons(ETH_P_IP) && fk.basic.n_proto != htons(ETH_P_IPV6)) {
+		rc = -EPROTONOSUPPORT;
+		goto out_clear;
+	}
+	if (fk.control.flags & FLOW_DIS_IS_FRAGMENT) {
+		rc = -EPROTONOSUPPORT;
+		goto out_clear;
+	}
 
+	req = efx->rps_slot + slot_idx;
 	efx_filter_init_rx(&req->spec, EFX_FILTER_PRI_HINT,
 			   efx->rx_scatter ? EFX_FILTER_FLAG_RX_SCATTER : 0,
 			   rxq_index);
@@ -933,6 +932,9 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 	req->flow_id = flow_id;
 	schedule_work(&req->work);
 	return 0;
+out_clear:
+	clear_bit(slot_idx, &efx->rps_slot_map);
+	return rc;
 }
 
 bool __efx_filter_rfs_expire(struct efx_nic *efx, unsigned int quota)

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Thu, 12 Apr 2018 15:02:50 +0100

> A misconfigured system (e.g. with all interrupts affinitised to all CPUs)
>  may produce a storm of ARFS steering events.  With the existing sfc ARFS
>  implementation, that could create a backlog of workitems that grinds the
>  system to a halt.  To prevent this, limit the number of workitems that
>  may be in flight for a given SFC device to 8 (EFX_RPS_MAX_IN_FLIGHT), and
>  return EBUSY from our ndo_rx_flow_steer method if the limit is reached.
> Given this limit, also store the workitems in an array of slots within the
>  struct efx_nic, rather than dynamically allocating for each request.
> 
> Signed-off-by: Edward Cree <ecree@solarflare.com>

I don't think this behavior is all that great.

If you really have to queue up these operations because they take a long
time, I think it is better to enter a synchronous mode and sleep once
you hit this in-flight limit of 8.

Either that or make the expiration work smarter when it has lots of events
to process.

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[Edward Cree]

On 12/04/18 16:11, David Miller wrote:
> From: Edward Cree <ecree@solarflare.com>
> Date: Thu, 12 Apr 2018 15:02:50 +0100
>
>> A misconfigured system (e.g. with all interrupts affinitised to all CPUs)
>>  may produce a storm of ARFS steering events.  With the existing sfc ARFS
>>  implementation, that could create a backlog of workitems that grinds the
>>  system to a halt.  To prevent this, limit the number of workitems that
>>  may be in flight for a given SFC device to 8 (EFX_RPS_MAX_IN_FLIGHT), and
>>  return EBUSY from our ndo_rx_flow_steer method if the limit is reached.
>> Given this limit, also store the workitems in an array of slots within the
>>  struct efx_nic, rather than dynamically allocating for each request.
>>
>> Signed-off-by: Edward Cree <ecree@solarflare.com>
> I don't think this behavior is all that great.
>
> If you really have to queue up these operations because they take a long
> time, I think it is better to enter a synchronous mode and sleep once
> you hit this in-flight limit of 8.
I don't think we can sleep at this point, ndo_rx_flow_steer is called from
 the RX path (netif_receive_skb_internal() -> get_rps_cpu() ->
 set_rps_cpu()).

> Either that or make the expiration work smarter when it has lots of events
> to process.
I'm afraid I don't understand what you mean here.
This code is not handling expiration of old ARFS filters, it's inserting
 new ones.

-Ed

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Thu, 12 Apr 2018 16:24:46 +0100

> This code is not handling expiration of old ARFS filters, it's inserting
>  new ones.

Then simply make the work process a queue, and add entries to the queue
here if the work is already scheduled.

Is there a reason why that wouldn't work?

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[Edward Cree]

It turns out this may all be moot anyway: I figured out why I was seeing
 ARFS storms and it wasn't the configuration issue I originally blamed.
My current ndo_rx_flow_steer() implementation, efx_filter_rfs(), returns
 0 for success, but the caller expects a filter ID to be returned (which
 we can't give it because we don't know what the filter ID will be until
 we start mucking around in the software state that's now protected by a
 sleepable lock).
As a result, when we call rps_may_expire_flow(), and pass it the _actual_
 filter ID, this doesn't match the one set_rps_cpu() recorded, so the
 function returns true and we immediately expire the filter.  Then the
 next packet to come along isn't steered, so ARFS asks us to insert a
 steering filter again.
As a quick fix I've simply tried making the rps_may_expire_flow() calls
 also pass a filter ID of 0, which prevents the ARFS storms.  This is
 safe; it may cause us to delay expiring a filter when flow_ids collide,
 but that can happen anyway with other drivers' implementations (e.g.
 mlx4 and mlx5 can potentially reuse filter IDs) so I presume it is OK.
I'll post a v2 with that fix in place of this Patch #2 shortly, then try
 to follow up with a counter-generated ID (similar to what mlx have).

-Ed

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Fri, 13 Apr 2018 13:36:28 +0100

> It turns out this may all be moot anyway: I figured out why I was seeing
>  ARFS storms and it wasn't the configuration issue I originally blamed.
> My current ndo_rx_flow_steer() implementation, efx_filter_rfs(), returns
>  0 for success, but the caller expects a filter ID to be returned (which
>  we can't give it because we don't know what the filter ID will be until
>  we start mucking around in the software state that's now protected by a
>  sleepable lock).
> As a result, when we call rps_may_expire_flow(), and pass it the _actual_
>  filter ID, this doesn't match the one set_rps_cpu() recorded, so the
>  function returns true and we immediately expire the filter.  Then the
>  next packet to come along isn't steered, so ARFS asks us to insert a
>  steering filter again.
> As a quick fix I've simply tried making the rps_may_expire_flow() calls
>  also pass a filter ID of 0, which prevents the ARFS storms.  This is
>  safe; it may cause us to delay expiring a filter when flow_ids collide,
>  but that can happen anyway with other drivers' implementations (e.g.
>  mlx4 and mlx5 can potentially reuse filter IDs) so I presume it is OK.
> I'll post a v2 with that fix in place of this Patch #2 shortly, then try
>  to follow up with a counter-generated ID (similar to what mlx have).

I understand the constraints you are working under, but do realize
that the real root of the problems is that you are implementing what
is defined clearly as a synchronous operation as asynchronous.

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[Edward Cree]

On 13/04/18 13:36, Edward Cree wrote:
> It turns out this may all be moot anyway: I figured out why I was seeing
>  ARFS storms and it wasn't the configuration issue I originally blamed.
Hmm, correction, while the fix I mentioned in my previous email is needed,
 it doesn't prevent the ARFS storms (although seems to lessen their
 severity, given that the machine didn't actually fall over this time),
 so we do also need some kind of limiting.

On 12/04/18 16:33, David Miller wrote:
> Then simply make the work process a queue, and add entries to the queue
> here if the work is already scheduled.
>
> Is there a reason why that wouldn't work?
That has the same problem as the existing code, that the length of the queue
 can grow without bound, potentially causing a very long lag between the
 request and its execution.  This then can quickly become exponential as,
 while waiting for the filter to be inserted, further packets from the same
 flow are received (still unsteered) and trigger duplicate ARFS requests
 (though I suppose it would be possible to scan the queue for matching flow
 IDs; but the concurrency / locking problems with that are 'interesting').

I'm not sure why you object to the dropping of requests - it seems reasonable
 to me to treat ARFS as a 'best-effort' thing.  The packets will still be
 received (just not necessarily on the core nearest the application), and if
 the flow continues it will generate more steering requests after the ones
 currently in flight have been processed.
And in practice we only get into this situation in the first place when we
 have interrupt affinities configured in such a way as to make ARFS
 practically useless anyway, so our failure to insert the filters is not of
 great significance.

On 13/04/18 15:45, David Miller wrote:
> I understand the constraints you are working under, but do realize
> that the real root of the problems is that you are implementing what
> is defined clearly as a synchronous operation as asynchronous.
Yes, it is unfortunate that we are unable to perform synchronous filter
 insertions, but you go to war with the hardware you have :(

-Ed

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Fri, 13 Apr 2018 15:52:25 +0100

> On 13/04/18 15:45, David Miller wrote:
>> I understand the constraints you are working under, but do realize
>> that the real root of the problems is that you are implementing what
>> is defined clearly as a synchronous operation as asynchronous.
> Yes, it is unfortunate that we are unable to perform synchronous filter
>  insertions, but you go to war with the hardware you have :(

Whilst you may not be able to program the filter into the hardware
synchronously, you should be able to allocate the ID and get all of
the software state setup.

At least then you can return the proper return value from the netdev
op.

People really aren't going to be happy if their performance goes into
the tank because their filter update rate, for whatever reason, hits
this magic backlog limit.

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[Edward Cree]

On 13/04/18 16:03, David Miller wrote:
> Whilst you may not be able to program the filter into the hardware
> synchronously, you should be able to allocate the ID and get all of
> the software state setup.
That's what we were doing before commit 3af0f34290f6 ("sfc: replace
 asynchronous filter operations"), and (as mentioned in that commit)
 that leads to (or at least the scheme we used had) race conditions
 which I could not see a way to fix.  If the hardware resets (and
 thus forgets all its filters) after we've done the software state
 setup but before we reach the point of finalising the software state
 after the hardware operation, we don't know what operations we need
 to do to re-apply the software state to the hardware, because we
 don't know whether the reset happened before or after the hardware
 operation.
Actually, it's not the insertion itself that this can happen to - if
 the reset happens first then the insert will fail because other
 hardware state we reference isn't set up yet.  However, inserting
 one filter can necessitate removing some others (lower-priority
 multicast filters for the same flow); the code before 3af0f34290f6
 was actually capable of getting so confused that it could double-
 free a pointer.
This all gets sufficiently complicated that even if I can find a
 locking scheme that in theory gets it right, there's pretty much a
 100% chance that the actual implementation will contain bugs,
 probably very subtle ones that can't reliably be reproduced etc.
 All my instincts say to get away from that if at all possible.

> People really aren't going to be happy if their performance goes into
> the tank because their filter update rate, for whatever reason, hits
> this magic backlog limit.
Well, the alternative, even if the software state setup part _could_
 be made synchronous, is to allow a potentially unbounded queue for
 the hardware update part (I think there are even still cases in
 which the exponential growth pathology is possible), causing the
 filter insertions to be delayed an arbitrarily long time.  Either
 the flow is still going by that time (in which case the backlog
 limit approach will get a new ndo_rx_flow_steer request and insert
 the filter too) or it isn't, in which case getting round to it
 eventually is no better than dropping it immediately.  In fact it's
 worse because now you waste time inserting a useless filter which
 delays new requests even more.
Besides, I'm fairly confident that the only cases in which you'll
 even come close to hitting the limit are ones where ARFS wouldn't
 do you much good anyway, such as:
* Misconfigured interrupt affinities where ARFS is entirely pointless
* Many short-lived flows (which greatly diminish the utility of ARFS)

So for multiple reasons, hitting the limit won't actually make
 performance worse, although it will often be a sign that performance
 will be bad for other reasons.

-Ed

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Fri, 13 Apr 2018 16:59:07 +0100

> On 13/04/18 16:03, David Miller wrote:
>> Whilst you may not be able to program the filter into the hardware
>> synchronously, you should be able to allocate the ID and get all of
>> the software state setup.
> That's what we were doing before commit 3af0f34290f6 ("sfc: replace
>  asynchronous filter operations"), and (as mentioned in that commit)
>  that leads to (or at least the scheme we used had) race conditions
>  which I could not see a way to fix.  If the hardware resets (and
>  thus forgets all its filters) after we've done the software state
>  setup but before we reach the point of finalising the software state
>  after the hardware operation, we don't know what operations we need
>  to do to re-apply the software state to the hardware, because we
>  don't know whether the reset happened before or after the hardware
>  operation.

When an entry is successfully programmed into the chip, you update
the software state.

When the chip resets, you clear all of those state booleans to false.

Indeed, you would have to synchronize these things somehow.

Is the issue that you learn about the hardware reset asynchronously,
and therefore cannot determine if filter insertion programming
happened afterwards and thus is still in the chip?

You must have a table of all the entries, so that you can reprogram
the hardware should it reset.  Or do you not handle things that way
and it's a lossy system?

> Well, the alternative, even if the software state setup part _could_
>  be made synchronous, is to allow a potentially unbounded queue for
>  the hardware update part (I think there are even still cases in
>  which the exponential growth pathology is possible), causing the
>  filter insertions to be delayed an arbitrarily long time.  Either
>  the flow is still going by that time (in which case the backlog
>  limit approach will get a new ndo_rx_flow_steer request and insert
>  the filter too) or it isn't, in which case getting round to it
>  eventually is no better than dropping it immediately.  In fact it's
>  worse because now you waste time inserting a useless filter which
>  delays new requests even more.
> Besides, I'm fairly confident that the only cases in which you'll
>  even come close to hitting the limit are ones where ARFS wouldn't
>  do you much good anyway, such as:
> * Misconfigured interrupt affinities where ARFS is entirely pointless
> * Many short-lived flows (which greatly diminish the utility of ARFS)
> 
> So for multiple reasons, hitting the limit won't actually make
>  performance worse, although it will often be a sign that performance
>  will be bad for other reasons.

Understood, thanks for explaining.

Please respin your series with the updates you talked about and I'll
apply it.

But generally we do have this issue with various kinds of
configuration programming and async vs. sync.

Re: [PATCH net 2/2] sfc: limit ARFS workitems in flight per channel

[Edward Cree]

On 13/04/18 17:14, David Miller wrote:
> Is the issue that you learn about the hardware reset asynchronously,
> and therefore cannot determine if filter insertion programming
> happened afterwards and thus is still in the chip?
Yes, pretty much.

> You must have a table of all the entries, so that you can reprogram
> the hardware should it reset.
Yes, we do have such a table; 'reprogram the hardware' happens in
 efx_ef10_filter_table_restore().

> Understood, thanks for explaining.
>
> Please respin your series with the updates you talked about and I'll
> apply it.
Will do, thanks.

-Ed