On Tue, May 28, 2024 at 03:48:37PM +0200, Alexander Lobakin wrote:
> Currently, sizeof(struct idpf_queue) is 32 Kb.
> This is due to the 12-bit hashtable declaration at the end of the queue.
> This HT is needed only for Tx queues when the flow scheduling mode is
> enabled. But &idpf_queue is unified for all of the queue types,
> provoking excessive memory usage.
> The unified structure in general makes the code less effective via
> suboptimal fields placement. You can't avoid that unless you make unions
> each 2 fields. Even then, different field alignment etc., doesn't allow
> you to optimize things to the limit.
> Split &idpf_queue into 4 structures corresponding to the queue types:
> RQ (Rx queue), SQ (Tx queue), FQ (buffer queue), and CQ (completion
> queue). Place only needed fields there and shortcuts handy for hotpath.
> Allocate the abovementioned hashtable dynamically and only when needed,
> keeping &idpf_tx_queue relatively short (192 bytes, same as Rx). This HT
> is used only for OOO completions, which aren't really hotpath anyway.
> Note that this change must be done atomically, otherwise it's really
> easy to get lost and miss something.
>
> Signed-off-by: Alexander Lobakin <aleksander.loba...@intel.com>
...
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
...
> @@ -1158,20 +1325,22 @@ static void idpf_rxq_set_descids(struct idpf_vport
> *vport, struct idpf_queue *q)
> */
> static int idpf_txq_group_alloc(struct idpf_vport *vport, u16 num_txq)
> {
> - bool flow_sch_en;
> - int err, i;
> + bool split, flow_sch_en;
> + int i;
>
> vport->txq_grps = kcalloc(vport->num_txq_grp,
> sizeof(*vport->txq_grps), GFP_KERNEL);
> if (!vport->txq_grps)
> return -ENOMEM;
>
> + split = idpf_is_queue_model_split(vport->txq_model);
> flow_sch_en = !idpf_is_cap_ena(vport->adapter, IDPF_OTHER_CAPS,
> VIRTCHNL2_CAP_SPLITQ_QSCHED);
>
> for (i = 0; i < vport->num_txq_grp; i++) {
> struct idpf_txq_group *tx_qgrp = &vport->txq_grps[i];
> struct idpf_adapter *adapter = vport->adapter;
> + struct idpf_txq_stash *stashes;
> int j;
>
> tx_qgrp->vport = vport;
> @@ -1180,45 +1349,62 @@ static int idpf_txq_group_alloc(struct idpf_vport
> *vport, u16 num_txq)
> for (j = 0; j < tx_qgrp->num_txq; j++) {
> tx_qgrp->txqs[j] = kzalloc(sizeof(*tx_qgrp->txqs[j]),
> GFP_KERNEL);
> - if (!tx_qgrp->txqs[j]) {
> - err = -ENOMEM;
> + if (!tx_qgrp->txqs[j])
> goto err_alloc;
> - }
> + }
> +
> + if (split && flow_sch_en) {
> + stashes = kcalloc(num_txq, sizeof(*stashes),
> + GFP_KERNEL);
Hi Alexander,
Here stashes is assigned a memory allocation and
then then assigned to tx_qgrp->stashes a few lines below...
> + if (!stashes)
> + goto err_alloc;
> +
> + tx_qgrp->stashes = stashes;
> }
>
> for (j = 0; j < tx_qgrp->num_txq; j++) {
> - struct idpf_queue *q = tx_qgrp->txqs[j];
> + struct idpf_tx_queue *q = tx_qgrp->txqs[j];
>
> q->dev = &adapter->pdev->dev;
> q->desc_count = vport->txq_desc_count;
> q->tx_max_bufs = idpf_get_max_tx_bufs(adapter);
> q->tx_min_pkt_len = idpf_get_min_tx_pkt_len(adapter);
> - q->vport = vport;
> + q->netdev = vport->netdev;
> q->txq_grp = tx_qgrp;
> - hash_init(q->sched_buf_hash);
>
> - if (flow_sch_en)
> - set_bit(__IDPF_Q_FLOW_SCH_EN, q->flags);
> + if (!split) {
> + q->clean_budget = vport->compln_clean_budget;
> + idpf_queue_assign(CRC_EN, q,
> + vport->crc_enable);
> + }
> +
> + if (!flow_sch_en)
> + continue;
> +
> + if (split) {
... but here elements of stashes seem to be assigned to q->stash
without stashes having being initialised.
Flagged by Smatch
> + q->stash = &stashes[j];
> + hash_init(q->stash->sched_buf_hash);
> + }
> +
> + idpf_queue_set(FLOW_SCH_EN, q);
> }
>
> - if (!idpf_is_queue_model_split(vport->txq_model))
> + if (!split)
> continue;
>
> tx_qgrp->complq = kcalloc(IDPF_COMPLQ_PER_GROUP,
> sizeof(*tx_qgrp->complq),
> GFP_KERNEL);
> - if (!tx_qgrp->complq) {
> - err = -ENOMEM;
> + if (!tx_qgrp->complq)
> goto err_alloc;
> - }
>
> - tx_qgrp->complq->dev = &adapter->pdev->dev;
> tx_qgrp->complq->desc_count = vport->complq_desc_count;
> - tx_qgrp->complq->vport = vport;
> tx_qgrp->complq->txq_grp = tx_qgrp;
> + tx_qgrp->complq->netdev = vport->netdev;
> + tx_qgrp->complq->clean_budget = vport->compln_clean_budget;
>
> if (flow_sch_en)
> - __set_bit(__IDPF_Q_FLOW_SCH_EN, tx_qgrp->complq->flags);
> + idpf_queue_set(FLOW_SCH_EN, tx_qgrp->complq);
> }
>
> return 0;
> @@ -1226,7 +1412,7 @@ static int idpf_txq_group_alloc(struct idpf_vport
> *vport, u16 num_txq)
> err_alloc:
> idpf_txq_group_rel(vport);
>
> - return err;
> + return -ENOMEM;
> }
>
> /**
...
