Re: [dpdk-dev] [dpdk-dev v3] [PATCH] net/octeontx2: add devargs to lock Rx/Tx ctx

["Andrzej Ostruszka [C]" <aostruszka@marvell.com>]

On 3/26/20 7:33 AM, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> Add device arguments to lock Rx/Tx contexts.
> Application can either choose to lock Rx or Tx contexts by using
> 'lock_rx_ctx' or 'lock_tx_ctx' respectively per each port.
> 
> Example:
> 	-w 0002:02:00.0,lock_rx_ctx=1 -w 0002:03:00.0,lock_tx_ctx=1
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>  v3 Changes:
>  - Split series into individual patches as targets are different.

You might need to insert also some "Depends-on:" or something like that
to mark that this patch depends on common changes in the other one.  I'm
not sure how this should work when one is dedicated to master and one
for next-marvell.

[...]
> diff --git a/drivers/net/octeontx2/otx2_ethdev.c b/drivers/net/octeontx2/otx2_ethdev.c
> index e60f4901c..6369c2fa9 100644
> --- a/drivers/net/octeontx2/otx2_ethdev.c
> +++ b/drivers/net/octeontx2/otx2_ethdev.c
> @@ -381,6 +381,40 @@ nix_cq_rq_init(struct rte_eth_dev *eth_dev, struct otx2_eth_dev *dev,
>  		goto fail;
>  	}
> 
> +	if (dev->lock_rx_ctx) {
> +		aq = otx2_mbox_alloc_msg_nix_aq_enq(mbox);
> +		aq->qidx = qid;
> +		aq->ctype = NIX_AQ_CTYPE_CQ;
> +		aq->op = NIX_AQ_INSTOP_LOCK;
> +
> +		aq = otx2_mbox_alloc_msg_nix_aq_enq(mbox);
> +		if (!aq) {
> +			/* The shared memory buffer can be full.
> +			 * Flush it and retry
> +			 */
> +			otx2_mbox_msg_send(mbox, 0);
> +			rc = otx2_mbox_wait_for_rsp(mbox, 0);
> +			if (rc < 0) {
> +				otx2_err("Failed to LOCK cq context");
> +				goto fail;

This fail doesn't do anything interesting so I would remove it and
replace all "goto fail" with "return rc".  That way you would be
consistent (e.g. below you return -ENOMEM).  Just like you do in
nix_cq_rq_uninit() - below.

> +			}
> +
> +			aq = otx2_mbox_alloc_msg_nix_aq_enq(mbox);
> +			if (!aq) {
> +				otx2_err("Failed to LOCK rq context");
> +				return -ENOMEM;
> +			}
> +		}
> +		aq->qidx = qid;
> +		aq->ctype = NIX_AQ_CTYPE_RQ;
> +		aq->op = NIX_AQ_INSTOP_LOCK;
> +		rc = otx2_mbox_process(mbox);
> +		if (rc < 0) {
> +			otx2_err("Failed to LOCK rq context");
> +			goto fail;
> +		}
> +	}
> +
>  	return 0;
>  fail:
>  	return rc;
> @@ -430,6 +464,40 @@ nix_cq_rq_uninit(struct rte_eth_dev *eth_dev, struct otx2_eth_rxq *rxq)
>  		return rc;
>  	}
> 
> +	if (dev->lock_rx_ctx) {
> +		aq = otx2_mbox_alloc_msg_nix_aq_enq(mbox);
> +		aq->qidx = rxq->rq;
> +		aq->ctype = NIX_AQ_CTYPE_CQ;
> +		aq->op = NIX_AQ_INSTOP_UNLOCK;
> +
> +		aq = otx2_mbox_alloc_msg_nix_aq_enq(mbox);
> +		if (!aq) {
> +			/* The shared memory buffer can be full.
> +			 * Flush it and retry
> +			 */
> +			otx2_mbox_msg_send(mbox, 0);
> +			rc = otx2_mbox_wait_for_rsp(mbox, 0);
> +			if (rc < 0) {
> +				otx2_err("Failed to UNLOCK cq context");
> +				return rc;
> +			}
> +
> +			aq = otx2_mbox_alloc_msg_nix_aq_enq(mbox);
> +			if (!aq) {
> +				otx2_err("Failed to UNLOCK rq context");
> +				return -ENOMEM;
> +			}
> +		}
> +		aq->qidx = rxq->rq;
> +		aq->ctype = NIX_AQ_CTYPE_RQ;
> +		aq->op = NIX_AQ_INSTOP_UNLOCK;
> +		rc = otx2_mbox_process(mbox);
> +		if (rc < 0) {
> +			otx2_err("Failed to UNLOCK rq context");
> +			return rc;
> +		}
> +	}
> +
>  	return 0;
>  }
[...]
> diff --git a/drivers/net/octeontx2/otx2_ethdev_devargs.c b/drivers/net/octeontx2/otx2_ethdev_devargs.c
> index 5390eb217..e8eba3d91 100644
> --- a/drivers/net/octeontx2/otx2_ethdev_devargs.c
> +++ b/drivers/net/octeontx2/otx2_ethdev_devargs.c
> @@ -124,6 +124,8 @@ parse_switch_header_type(const char *key, const char *value, void *extra_args)
>  #define OTX2_FLOW_MAX_PRIORITY "flow_max_priority"
>  #define OTX2_SWITCH_HEADER_TYPE "switch_header"
>  #define OTX2_RSS_TAG_AS_XOR "tag_as_xor"
> +#define OTX2_LOCK_RX_CTX "lock_rx_ctx"
> +#define OTX2_LOCK_TX_CTX "lock_tx_ctx"
> 
>  int
>  otx2_ethdev_parse_devargs(struct rte_devargs *devargs, struct otx2_eth_dev *dev)
> @@ -134,9 +136,11 @@ otx2_ethdev_parse_devargs(struct rte_devargs *devargs, struct otx2_eth_dev *dev)
>  	uint16_t switch_header_type = 0;
>  	uint16_t flow_max_priority = 3;
>  	uint16_t ipsec_in_max_spi = 1;
> -	uint16_t scalar_enable = 0;
>  	uint16_t rss_tag_as_xor = 0;
> +	uint16_t scalar_enable = 0;
>  	struct rte_kvargs *kvlist;
> +	uint8_t lock_rx_ctx = 0;
> +	uint8_t lock_tx_ctx = 0;

I missed that previously, but these needs to be uint16_t.  This is
because you call parse_flag() which is treating its extra_arg as pointer
to uint16_t.

>  	if (devargs == NULL)
>  		goto null_devargs;
> @@ -161,6 +165,10 @@ otx2_ethdev_parse_devargs(struct rte_devargs *devargs, struct otx2_eth_dev *dev)
>  			   &parse_switch_header_type, &switch_header_type);
>  	rte_kvargs_process(kvlist, OTX2_RSS_TAG_AS_XOR,
>  			   &parse_flag, &rss_tag_as_xor);
> +	rte_kvargs_process(kvlist, OTX2_LOCK_RX_CTX,
> +			   &parse_flag, &lock_rx_ctx);
> +	rte_kvargs_process(kvlist, OTX2_LOCK_TX_CTX,
> +			   &parse_flag, &lock_tx_ctx);
>  	otx2_parse_common_devargs(kvlist);
>  	rte_kvargs_free(kvlist);
[...]

With that uint16_t fix above:
Reviewed-by: Andrzej Ostruszka <aostruszka@marvell.com>

With regards
Andrzej Ostruszka