Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 3
- [Low] Three comments (and the commit message) state a shrink
precondition that this patch itself violates, and one pre-existing…
- [Low] mana_free_qset() gains a leading `struct mana_port_context *apc`
parameter (exported prototype in include/net/mana/mana.h plus eight…
- [Low] Concern (assessed as a false positive): the commit lacks a Fixes:
trailer for the commit that made the grow-only EQ pool reachable.
--
net: mana: release EQs left idle by a channel-count reduction
The shared EQ pool previously only grew, so a port kept the high-water mark
of every channel count it had ever used. This adds mana_shrink_eqs() to
release the EQs above the live queue count once a retiring set is gone, and
fixes mana_create_eq_debugfs() to record the dentry in apc->eqs[i] so a
single EQ's debugfs directory can be removed.
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 69e34fab8fe8e..2295769da0306 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -2084,6 +2085,37 @@ static int mana_grow_eqs(struct mana_port_context
> *apc, unsigned int need)
> return err;
> }
>
> +/* Release EQs above @keep, returning the MSI-X vectors freed. Only safe once
> + * no set references them, i.e. after mana_free_qset(), or a live CQ would
> + * point at a destroyed EQ.
> + */
[Severity: Low]
This comment says a shrink is only safe after mana_free_qset(), and the
commit message says the same:
"Release the EQs above the live queue count once a retiring set has been
torn down. That is the only safe point"
The same patch also calls mana_shrink_eqs() from mana_alloc_qset()'s
out_err path, which is not after mana_free_qset(). Can the two be
reconciled?
The rule that appears to hold at both sites is "no CQ of any set
references eqs[keep..num_eqs-1]": mana_alloc_qset()'s out_err is reached
either before any queue was built, or after mana_destroy_rxqs(scratch) and
mana_destroy_txq(scratch) destroyed the CQs that attempt created. Would
stating that condition instead be more accurate, here and in
mana_free_qset()'s "Safe here and only here" comment below?
Separately, the error exit of mana_grow_eqs() just above this new helper
still reads:
out:
/* Keep whatever was created: the running queue set still needs
its
* own EQs, and the extras are reused by the next attempt.
*/
return err;
Since its only caller, mana_alloc_qset(), now runs
mana_shrink_eqs(apc, apc->num_queues) on that same error return, is
anything still kept for the next attempt?
> +static void mana_shrink_eqs(struct mana_port_context *apc, unsigned int keep)
> +{
> + struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
> + struct gdma_queue *eq;
> + unsigned int msi;
> + unsigned int i;
> +
> + if (!apc->eqs || keep >= apc->num_eqs)
> + return;
> +
> + for (i = keep; i < apc->num_eqs; i++) {
> + eq = apc->eqs[i].eq;
> + if (!eq)
> + continue;
> +
> + debugfs_remove_recursive(apc->eqs[i].mana_eq_debugfs);
> + apc->eqs[i].mana_eq_debugfs = NULL;
> +
> + msi = eq->eq.msix_index;
> + mana_gd_destroy_queue(gc, eq);
> + mana_gd_put_gic(gc, !gc->msi_sharing, msi);
> + apc->eqs[i].eq = NULL;
> + }
> +
> + apc->num_eqs = keep;
> +}
> +
> static int mana_fence_rq(struct mana_port_context *apc, struct mana_rxq *rxq)
> {
> struct mana_fence_rq_resp resp = {};
> @@ -4319,6 +4351,13 @@ int mana_alloc_qset(struct mana_port_context *apc,
> kfree(scratch->rxqs);
> scratch->rxqs = NULL;
> out_err:
> + /* Give back any EQ this attempt added to the shared pool rather than
> + * holding its MSI-X vectors until some later teardown: the live set
> + * still needs only apc->num_queues of them. Safe here because this
> + * set's CQs have already been destroyed above.
> + */
> + mana_shrink_eqs(apc, apc->num_queues);
> +
> netdev_err(ndev, "%s(num_queues=%u) failed: %d\n", __func__,
> num_queues, err);
> return err;
> @@ -4649,7 +4688,8 @@ static void mana_qset_debugfs_publish(struct
> mana_port_context *apc)
> /* Tear down @qset, no longer installed on @apc, against @scratch so the live
> * context never points at queues being freed.
> */
> -void mana_free_qset(struct mana_port_context *scratch, struct mana_qset
> *qset)
> +void mana_free_qset(struct mana_port_context *apc,
> + struct mana_port_context *scratch, struct mana_qset *qset)
> {
> struct bpf_prog *retiring_prog;
> unsigned int retiring_queues;
[Severity: Low]
Does the new apc parameter carry anything the function cannot already
derive? mana_qset_scratch_alloc() does *scratch = *apc, so
netdev_priv(scratch->ndev) == apc for every one of the eight call sites,
which is exactly what this patch relies on when it replaces
mana_qset_debugfs_publish(netdev_priv(scratch->ndev)) with
mana_qset_debugfs_publish(apc).
The TX-drain-failure block inside the same function still has:
if (mana_drain_txqs(scratch)) {
...
struct mana_port_context *apc = netdev_priv(scratch->ndev);
so that local now shadows the new parameter, which -Wshadow / W=2 builds
will flag.
Nothing verifies that a caller passes a matching (apc, scratch) pair, and a
mismatched pair would let the mana_shrink_eqs() call below destroy another
port's EQs. Would dropping either the parameter or the inner declaration
be preferable?
The exported prototype in include/net/mana/mana.h changes with it - is that
signature change worth a line in the commit message?
> @@ -4745,12 +4785,19 @@ void mana_free_qset(struct mana_port_context
> *scratch, struct mana_qset *qset)
>
> memset(qset, 0, sizeof(*qset));
>
> + /* This set is gone, so any EQ above the live queue count is now
> + * unreferenced. Release those vectors instead of holding them at the
> + * high-water mark. Safe here and only here: the retiring set's CQs
> + * have just been destroyed.
> + */
> + mana_shrink_eqs(apc, apc->num_queues);
> +
> /* Queues built through a scratch context carry no debugfs nodes,
> * because both sets are alive during the swap and would collide on
> * the same names. The retiring set's nodes are gone now, so the
> * published queues can finally take those names.
> */
> - mana_qset_debugfs_publish(netdev_priv(scratch->ndev));
> + mana_qset_debugfs_publish(apc);
> }
>
> /* --- end of pre-allocate + swap reconfiguration path
> ---------------------- */
> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index 9a43856760776..140948b2b45d5 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h
> @@ -797,7 +797,8 @@ int mana_alloc_qset(struct mana_port_context *apc,
> int mana_publish_qset(struct mana_port_context *apc, struct mana_qset *newq,
> struct mana_qset *out_old);
> void mana_publish_close_if_needed(struct mana_port_context *apc);
> -void mana_free_qset(struct mana_port_context *scratch, struct mana_qset
> *qset);
> +void mana_free_qset(struct mana_port_context *apc,
> + struct mana_port_context *scratch, struct mana_qset *qset);
Both observations above are documentation and readability points; I did not
find a path where either shrink call site touches an EQ that a live CQ
still references.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901014442.2945689-1-longli%40microsoft.com
