Re: [net-next Patch V4 4/4] octeontx2-pf: Add support for HTB offload

From: Simon Horman
Date: Fri Feb 10 2023 - 10:06:23 EST

On Fri, Feb 10, 2023 at 04:40:51PM +0530, Hariprasad Kelam wrote:
> From: Naveen Mamindlapalli <naveenm@xxxxxxxxxxx>
>
> This patch registers callbacks to support HTB offload.
>
> Below are features supported,
>
> - supports traffic shaping on the given class by honoring rate and ceil
> configuration.
>
> - supports traffic scheduling, which prioritizes different types of
> traffic based on strict priority values.
>
> - supports the creation of leaf to inner classes such that parent node
> rate limits apply to all child nodes.
>
> Signed-off-by: Naveen Mamindlapalli <naveenm@xxxxxxxxxxx>
> Signed-off-by: Hariprasad Kelam <hkelam@xxxxxxxxxxx>
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>
> ---
> .../ethernet/marvell/octeontx2/af/common.h | 2 +-
> .../ethernet/marvell/octeontx2/nic/Makefile | 2 +-
> .../marvell/octeontx2/nic/otx2_common.c | 37 +-
> .../marvell/octeontx2/nic/otx2_common.h | 7 +
> .../marvell/octeontx2/nic/otx2_ethtool.c | 31 +-
> .../ethernet/marvell/octeontx2/nic/otx2_pf.c | 47 +-
> .../ethernet/marvell/octeontx2/nic/otx2_reg.h | 13 +
> .../ethernet/marvell/octeontx2/nic/otx2_tc.c | 7 +-
> .../net/ethernet/marvell/octeontx2/nic/qos.c | 1541 +++++++++++++++++
> .../net/ethernet/marvell/octeontx2/nic/qos.h | 56 +-
> .../ethernet/marvell/octeontx2/nic/qos_sq.c | 20 +-
> include/net/sch_generic.h | 2 +
> net/sched/sch_generic.c | 5 +-
> 13 files changed, 1741 insertions(+), 29 deletions(-)
> create mode 100644 drivers/net/ethernet/marvell/octeontx2/nic/qos.c

nit: this patch is rather long

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 4cb3fab8baae..5653b06d9dd8 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c

...

> +int otx2_txschq_stop(struct otx2_nic *pfvf)
> +{
> + int lvl, schq;
> +
> + /* free non QOS TLx nodes */
> + for (lvl = 0; lvl < NIX_TXSCH_LVL_CNT; lvl++)
> + otx2_txschq_free_one(pfvf, lvl,
> + pfvf->hw.txschq_list[lvl][0]);
>
> /* Clear the txschq list */
> for (lvl = 0; lvl < NIX_TXSCH_LVL_CNT; lvl++) {
> for (schq = 0; schq < MAX_TXSCHQ_PER_FUNC; schq++)
> pfvf->hw.txschq_list[lvl][schq] = 0;
> }
> - return err;
> +
> + return 0;

nit: This function always returns 0. Perhaps it's return value could be null
and the error handling code at the call sites removed.

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/qos.c b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> new file mode 100644
> index 000000000000..2d8189ece31d
> --- /dev/null
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c

...

> +int otx2_clean_qos_queues(struct otx2_nic *pfvf)
> +{
> + struct otx2_qos_node *root;
> +
> + root = otx2_sw_node_find(pfvf, OTX2_QOS_ROOT_CLASSID);
> + if (!root)
> + return 0;
> +
> + return otx2_qos_update_smq(pfvf, root, QOS_SMQ_FLUSH);
> +}

nit: It seems that the return code of this function is always ignored by
callers. Perhaps either the call sites should detect errors, or the
return type of this function should be changed to void.

> +
> +void otx2_qos_config_txschq(struct otx2_nic *pfvf)
> +{
> + struct otx2_qos_node *root;
> + int err;
> +
> + root = otx2_sw_node_find(pfvf, OTX2_QOS_ROOT_CLASSID);
> + if (!root)
> + return;
> +
> + err = otx2_qos_txschq_config(pfvf, root);
> + if (err) {
> + netdev_err(pfvf->netdev, "Error update txschq configuration\n");
> + goto root_destroy;
> + }
> +
> + err = otx2_qos_txschq_push_cfg_tl(pfvf, root, NULL);
> + if (err) {
> + netdev_err(pfvf->netdev, "Error update txschq configuration\n");
> + goto root_destroy;
> + }
> +
> + err = otx2_qos_update_smq(pfvf, root, QOS_CFG_SQ);
> + if (err) {
> + netdev_err(pfvf->netdev, "Error update smq configuration\n");
> + goto root_destroy;
> + }
> +
> + return;
> +
> +root_destroy:
> + otx2_qos_root_destroy(pfvf);
> +}

I am curious as to why the root is destroyed here.
But such cleanup doesn't apply to other places
where otx2_qos_txschq_config() is called.


...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/qos.h b/drivers/net/ethernet/marvell/octeontx2/nic/qos.h
> index ef8c99a6b2d0..d8e32a6e541d 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/qos.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/qos.h

...

> struct otx2_qos {
> + DECLARE_HASHTABLE(qos_hlist, order_base_2(OTX2_QOS_MAX_LEAF_NODES));
> + DECLARE_BITMAP(qos_sq_bmap, OTX2_QOS_MAX_LEAF_NODES);
> u16 qid_to_sqmap[OTX2_QOS_MAX_LEAF_NODES];
> + u16 maj_id;
> + u16 defcls;

On x86_64 there is a 4 byte hole here...

> + struct list_head qos_tree;
> + struct mutex qos_lock; /* child list lock */
> + u8 link_cfg_lvl; /* LINKX_CFG CSRs mapped to TL3 or TL2's index ? */

And link_cfg_lvl is on a cacheline all by itself.

I'm not sure if it makes any difference, but pehraps
it makes more sense to place link_cfg_lvl in the hole above.

$ pahole drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.o
...
struct otx2_qos {
struct hlist_head qos_hlist[16]; /* 0 128 */
/* --- cacheline 2 boundary (128 bytes) --- */
long unsigned int qos_sq_bmap[1]; /* 128 8 */
u16 qid_to_sqmap[16]; /* 136 32 */
u16 maj_id; /* 168 2 */
u16 defcls; /* 170 2 */

/* XXX 4 bytes hole, try to pack */

struct list_head qos_tree; /* 176 16 */
/* --- cacheline 3 boundary (192 bytes) --- */
struct mutex qos_lock; /* 192 160 */
/* --- cacheline 5 boundary (320 bytes) was 32 bytes ago --- */
u8 link_cfg_lvl; /* 352 1 */

/* size: 360, cachelines: 6, members: 8 */
/* sum members: 349, holes: 1, sum holes: 4 */
/* padding: 7 */
/* last cacheline: 40 bytes */
};
...

> +};
> +
> +struct otx2_qos_node {
> + /* htb params */
> + u32 classid;

On x86_64 there is a 4 byte hole here,

> + u64 rate;
> + u64 ceil;
> + u32 prio;
> + /* hw txschq */
> + u8 level;

a one byte hole here,

> + u16 schq;
> + u16 qid;
> + u16 prio_anchor;

another four byte hole here,

> + DECLARE_BITMAP(prio_bmap, OTX2_QOS_MAX_PRIO + 1);
> + /* list management */
> + struct hlist_node hlist;

the first cacheline ends here,

> + struct otx2_qos_node *parent; /* parent qos node */

And this is an 8 byte entity.

I'm not sure if it is advantagous,
but I think parent could be moved to the first cacheline.

> + struct list_head list;
> + struct list_head child_list;
> + struct list_head child_schq_list;
> };

$ pahole drivers/net/ethernet/marvell/octeontx2/nic/qos.o
...
struct otx2_qos_node {
u32 classid; /* 0 4 */

/* XXX 4 bytes hole, try to pack */

u64 rate; /* 8 8 */
u64 ceil; /* 16 8 */
u32 prio; /* 24 4 */
u8 level; /* 28 1 */

/* XXX 1 byte hole, try to pack */

u16 schq; /* 30 2 */
u16 qid; /* 32 2 */
u16 prio_anchor; /* 34 2 */

/* XXX 4 bytes hole, try to pack */

long unsigned int prio_bmap[1]; /* 40 8 */
struct hlist_node hlist; /* 48 16 */
/* --- cacheline 1 boundary (64 bytes) --- */
struct otx2_qos_node * parent; /* 64 8 */
struct list_head list; /* 72 16 */
struct list_head child_list; /* 88 16 */
struct list_head child_schq_list; /* 104 16 */

/* size: 120, cachelines: 2, members: 14 */
/* sum members: 111, holes: 3, sum holes: 9 */
/* last cacheline: 56 bytes */
};
...