From: Maciej Fijalkowski <[email protected]>
[ Upstream commit 296f13ff3854535009a185aaf8e3603266d39d94 ]
With the upcoming introduction of batching to XSK data path,
performance wise it will be the best to have the ring descriptor count
to be aligned to power of 2.
Check if ring sizes that user is going to attach the XSK socket fulfill
the condition above. For Tx side, although check is being done against
the Tx queue and in the end the socket will be attached to the XDP
queue, it is fine since XDP queues get the ring->count setting from Tx
queues.
Suggested-by: Alexander Lobakin <[email protected]>
Signed-off-by: Maciej Fijalkowski <[email protected]>
Signed-off-by: Daniel Borkmann <[email protected]>
Reviewed-by: Alexander Lobakin <[email protected]>
Acked-by: Magnus Karlsson <[email protected]>
Link: https://lore.kernel.org/bpf/[email protected]
Signed-off-by: Sasha Levin <[email protected]>
---
drivers/net/ethernet/intel/ice/ice_xsk.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 5581747947e57..0348cc4265034 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -321,6 +321,13 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool, u16 qid)
bool if_running, pool_present = !!pool;
int ret = 0, pool_failure = 0;
+ if (!is_power_of_2(vsi->rx_rings[qid]->count) ||
+ !is_power_of_2(vsi->tx_rings[qid]->count)) {
+ netdev_err(vsi->netdev, "Please align ring sizes to power of 2\n");
+ pool_failure = -EINVAL;
+ goto failure;
+ }
+
if_running = netif_running(vsi->netdev) && ice_is_xdp_ena_vsi(vsi);
if (if_running) {
@@ -343,6 +350,7 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool, u16 qid)
netdev_err(vsi->netdev, "ice_qp_ena error = %d\n", ret);
}
+failure:
if (pool_failure) {
netdev_err(vsi->netdev, "Could not %sable buffer pool, error = %d\n",
pool_present ? "en" : "dis", pool_failure);
--
2.35.1
On Mon, Aug 29, 2022 at 12:58:29PM +0200, Greg Kroah-Hartman wrote:
> From: Maciej Fijalkowski <[email protected]>
>
> [ Upstream commit 296f13ff3854535009a185aaf8e3603266d39d94 ]
>
> With the upcoming introduction of batching to XSK data path,
> performance wise it will be the best to have the ring descriptor count
> to be aligned to power of 2.
>
> Check if ring sizes that user is going to attach the XSK socket fulfill
> the condition above. For Tx side, although check is being done against
> the Tx queue and in the end the socket will be attached to the XDP
> queue, it is fine since XDP queues get the ring->count setting from Tx
> queues.
Hi Greg,
We had multiple customers reporting that this change makes them unable to
use max ring size which is 8160 for this particular driver (which is not a
power of 2 obviously) so we are about to send a patch that will drop this
limitation.
To avoid the double work, can you please not proceed with this one?
The other two:
ice: xsk: prohibit usage of non-balanced queue id
ice: xsk: use Rx rings XDP ring when picking NAPI context
are valid and needed.
FWIW this was a part of -next patch set, so I suppose you picked this due
to some dependency?
Thanks,
Maciej
>
> Suggested-by: Alexander Lobakin <[email protected]>
> Signed-off-by: Maciej Fijalkowski <[email protected]>
> Signed-off-by: Daniel Borkmann <[email protected]>
> Reviewed-by: Alexander Lobakin <[email protected]>
> Acked-by: Magnus Karlsson <[email protected]>
> Link: https://lore.kernel.org/bpf/[email protected]
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> drivers/net/ethernet/intel/ice/ice_xsk.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> index 5581747947e57..0348cc4265034 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> @@ -321,6 +321,13 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool, u16 qid)
> bool if_running, pool_present = !!pool;
> int ret = 0, pool_failure = 0;
>
> + if (!is_power_of_2(vsi->rx_rings[qid]->count) ||
> + !is_power_of_2(vsi->tx_rings[qid]->count)) {
> + netdev_err(vsi->netdev, "Please align ring sizes to power of 2\n");
> + pool_failure = -EINVAL;
> + goto failure;
> + }
> +
> if_running = netif_running(vsi->netdev) && ice_is_xdp_ena_vsi(vsi);
>
> if (if_running) {
> @@ -343,6 +350,7 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool, u16 qid)
> netdev_err(vsi->netdev, "ice_qp_ena error = %d\n", ret);
> }
>
> +failure:
> if (pool_failure) {
> netdev_err(vsi->netdev, "Could not %sable buffer pool, error = %d\n",
> pool_present ? "en" : "dis", pool_failure);
> --
> 2.35.1
>
>
>
On Mon, Aug 29, 2022 at 01:16:40PM +0200, Maciej Fijalkowski wrote:
> On Mon, Aug 29, 2022 at 12:58:29PM +0200, Greg Kroah-Hartman wrote:
> > From: Maciej Fijalkowski <[email protected]>
> >
> > [ Upstream commit 296f13ff3854535009a185aaf8e3603266d39d94 ]
> >
> > With the upcoming introduction of batching to XSK data path,
> > performance wise it will be the best to have the ring descriptor count
> > to be aligned to power of 2.
> >
> > Check if ring sizes that user is going to attach the XSK socket fulfill
> > the condition above. For Tx side, although check is being done against
> > the Tx queue and in the end the socket will be attached to the XDP
> > queue, it is fine since XDP queues get the ring->count setting from Tx
> > queues.
>
> Hi Greg,
>
> We had multiple customers reporting that this change makes them unable to
> use max ring size which is 8160 for this particular driver (which is not a
> power of 2 obviously) so we are about to send a patch that will drop this
> limitation.
>
> To avoid the double work, can you please not proceed with this one?
> The other two:
> ice: xsk: prohibit usage of non-balanced queue id
> ice: xsk: use Rx rings XDP ring when picking NAPI context
>
> are valid and needed.
This is in the 5.18 kernel release, which has been out for a while.
We will be glad to pick up the fixed commit when it hits Linus's tree.
> FWIW this was a part of -next patch set, so I suppose you picked this due
> to some dependency?
I think it was, for a later patch in the series, 5a42f112d367 ("ice:
xsk: prohibit usage of non-balanced queue id").
thanks,
greg k-h