Re: [PATCH rdma-next 10/23] RDMA/mlx5: Introduce per-CQ poll_one hook in mlx5_ib_cq

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message calls this "a pure refactor with no behaviour
  change", but it changes the per-CQE fast path.

--

RDMA/mlx5: Introduce per-CQ poll_one hook in mlx5_ib_cq

This adds a poll_one function pointer to struct mlx5_ib_cq. It is
initialized to mlx5_poll_one() in mlx5_ib_create_cq() and
mlx5_ib_create_user_cq(), and mlx5_ib_poll_cq() now dispatches through
it. A follow-up patch uses the hook to install a specialised poll_one for
the UMR CQ.

> diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
> index 55ffd7d4de517..11c1153747c24 100644
> --- a/drivers/infiniband/hw/mlx5/cq.c
> +++ b/drivers/infiniband/hw/mlx5/cq.c
> @@ -633,7 +633,7 @@ int mlx5_ib_poll_cq(struct ib_cq *ibcq, int num_entries, struct ib_wc *wc)
>  		soft_polled = poll_soft_wc(cq, num_entries, wc, false);
>  
>  	for (npolled = 0; npolled < num_entries - soft_polled; npolled++) {
> -		if (mlx5_poll_one(cq, &cur_qp, wc + soft_polled + npolled))
> +		if (cq->poll_one(cq, &cur_qp, wc + soft_polled + npolled))
>  			break;
>  	}

[Severity: Low]
The commit message says:

  This is a pure refactor with no behaviour change.

Does that hold for the per-CQE fast path?

Before this patch, mlx5_poll_one() was static and had one caller, this
loop in mlx5_ib_poll_cq(), so the compiler could inline it.

The patch stores its address in cq->poll_one in mlx5_ib_create_cq() and
mlx5_ib_create_user_cq(). As a result, every polled CQE on every mlx5 CQ
now goes through an indirect call. On kernels with Spectre-v2
mitigations, that call goes through a retpoline thunk.

Only the UMR CQ needs a different implementation, which the later commit
"RDMA/mlx5: Verify UMR CQ polling against a hostile NIC" installs. Even
so, every in-kernel consumer polling an mlx5 CQ (NVMe-oF, iSER, SRP,
NFS/RDMA, SMC-R, RDS, IPoIB) takes the indirect path:

ib_poll_cq()->mlx5_ib_poll_cq()->cq->poll_one()

The end of the series doesn't seem to restore a direct call for the
common case, and there is no INDIRECT_CALL_1() use under
drivers/infiniband/. Would something like this avoid the cost?

    if (INDIRECT_CALL_1(cq->poll_one, mlx5_poll_one,
                        cq, &cur_qp, wc + soft_polled + npolled))

The final mlx5_poll_one() also already has an
unlikely(mlx5_ib_adversarial_mode) check, which a cheap dispatch here
could reuse.

If the indirect call stays, could the commit message mention this
trade-off rather than say there is no behaviour change?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103163949.1554-1-yishaih%40nvidia.com