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

[Simon Horman <simon.horman@corigine.com>]

On Fri, Feb 10, 2023 at 04:40:51PM +0530, Hariprasad Kelam wrote:
> From: Naveen Mamindlapalli <naveenm@marvell.com>
> 
> 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@marvell.com>
> Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> ---
>  .../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 */
};
...