Re: [dpdk-dev] [PATCH v13 3/6] drivers/net: update Rx RSS hash offload capabilities

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

>On 10/25/19 5:33 PM, pbhagavatula@marvell.com wrote:
>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>
>> Add DEV_RX_OFFLOAD_RSS_HASH flag for all PMDs that support RSS
>hash
>> delivery.
>>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> Reviewed-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> Acked-by: Jerin Jacob <jerinj@marvell.com>
>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>
>[snip]
>
>> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
>b/drivers/net/bnxt/bnxt_ethdev.c
>> index e7ec99e15..d4f8cc92a 100644
>> --- a/drivers/net/bnxt/bnxt_ethdev.c
>> +++ b/drivers/net/bnxt/bnxt_ethdev.c
>> @@ -117,7 +117,8 @@ static const struct rte_pci_id
>bnxt_pci_id_map[] = {
>>   				     DEV_RX_OFFLOAD_KEEP_CRC | \
>>   				     DEV_RX_OFFLOAD_VLAN_EXTEND |
>\
>>   				     DEV_RX_OFFLOAD_TCP_LRO | \
>> -				     DEV_RX_OFFLOAD_SCATTER)
>> +				     DEV_RX_OFFLOAD_SCATTER | \
>> +				     DEV_RX_OFFLOAD_RSS_HASH)
>>
>>   static int bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int
>mask);
>>   static void bnxt_print_link_info(struct rte_eth_dev *eth_dev);
>> @@ -681,6 +682,12 @@ static int bnxt_dev_configure_op(struct
>rte_eth_dev *eth_dev)
>>   	bp->rx_cp_nr_rings = bp->rx_nr_rings;
>>   	bp->tx_cp_nr_rings = bp->tx_nr_rings;
>>
>> +	if (!(rx_offloads & DEV_RX_OFFLOAD_RSS_HASH)) {
>> +		PMD_DRV_LOG(INFO, "RX_OFFLOAD_RSS_HASH
>cannot be disabled\n");
>
>Shouldn't logging be done from rte_eth_dev_configure()?
>I.e. a generic function which is called after dev_configure callback and
>take a look at dev_conf->rx_mode.offloads and
>dev->data->dev_conf.rxmode.offloads and for each bit which differs
>log message using rte_eth_dev_rx_offload_name().
>Same for Tx while we are on the page. I.e. two more patch just before
>this one.
>

Just to be clear this log would effect all offloads which can't be disabled for
a give PMD.

>> +		rx_offloads |= DEV_RX_OFFLOAD_RSS_HASH;
>> +		eth_dev->data->dev_conf.rxmode.offloads =
>rx_offloads;
>> +	}
>> +
>>   	if (rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
>>   		eth_dev->data->mtu =
>>   			eth_dev->data-
>>dev_conf.rxmode.max_rx_pkt_len -
>
>[snip]
>
>> diff --git a/drivers/net/sfc/sfc_ef10_essb_rx.c
>b/drivers/net/sfc/sfc_ef10_essb_rx.c
>> index 63da807ea..220ef0e47 100644
>> --- a/drivers/net/sfc/sfc_ef10_essb_rx.c
>> +++ b/drivers/net/sfc/sfc_ef10_essb_rx.c
>> @@ -716,7 +716,7 @@ struct sfc_dp_rx sfc_ef10_essb_rx = {
>>   	.features		= SFC_DP_RX_FEAT_FLOW_FLAG |
>>   				  SFC_DP_RX_FEAT_FLOW_MARK,
>>   	.dev_offload_capa	= DEV_RX_OFFLOAD_CHECKSUM,
>> -	.queue_offload_capa	= 0,
>> +	.queue_offload_capa	= DEV_RX_OFFLOAD_RSS_HASH,
>
>Please, move it dev_offload_capa to be sure that it cannot
>be requested on queue level and no checks are required
>on queue level that the offload cannot be disabled.
>We'll move to queue level when/if it is really supported on queue level.
>

Sure will fix all sfc related comments in v14.

>>   	.get_dev_info		= sfc_ef10_essb_rx_get_dev_info,
>>   	.pool_ops_supported	=
>sfc_ef10_essb_rx_pool_ops_supported,
>>   	.qsize_up_rings		=
>sfc_ef10_essb_rx_qsize_up_rings,
>> diff --git a/drivers/net/sfc/sfc_ef10_rx.c
>b/drivers/net/sfc/sfc_ef10_rx.c
>> index f2fc6e70a..85b5df466 100644
>> --- a/drivers/net/sfc/sfc_ef10_rx.c
>> +++ b/drivers/net/sfc/sfc_ef10_rx.c
>> @@ -797,7 +797,8 @@ struct sfc_dp_rx sfc_ef10_rx = {
>>   				  SFC_DP_RX_FEAT_INTR,
>>   	.dev_offload_capa	= DEV_RX_OFFLOAD_CHECKSUM |
>>
>DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM,
>> -	.queue_offload_capa	= DEV_RX_OFFLOAD_SCATTER,
>> +	.queue_offload_capa	= DEV_RX_OFFLOAD_SCATTER |
>> +				  DEV_RX_OFFLOAD_RSS_HASH,
>
>Same here
>
>>   	.get_dev_info		= sfc_ef10_rx_get_dev_info,
>>   	.qsize_up_rings		= sfc_ef10_rx_qsize_up_rings,
>>   	.qcreate		= sfc_ef10_rx_qcreate,
>> diff --git a/drivers/net/sfc/sfc_ethdev.c
>b/drivers/net/sfc/sfc_ethdev.c
>> index 454b8956a..403711ca0 100644
>> --- a/drivers/net/sfc/sfc_ethdev.c
>> +++ b/drivers/net/sfc/sfc_ethdev.c
>> @@ -206,6 +206,11 @@ sfc_dev_configure(struct rte_eth_dev *dev)
>>   	sfc_log_init(sa, "entry n_rxq=%u n_txq=%u",
>>   		     dev_data->nb_rx_queues, dev_data-
>>nb_tx_queues);
>>
>> +	if (!(dev_data->dev_conf.rxmode.offloads &
>DEV_RX_OFFLOAD_RSS_HASH)) {
>> +		sfc_info(sa, "RX_OFFLOAD_RSS_HASH cannot be
>disabled");
>> +		dev_data->dev_conf.rxmode.offloads |=
>DEV_RX_OFFLOAD_RSS_HASH;
>> +	}
>> +
>
>It should be in drivers/net/sfc/sfc_rx.c sfc_rx_check_mode() close to
>the
>end of the function and similar to DEV_RX_OFFLOAD_CHECKSUM.
>
>>   	sfc_adapter_lock(sa);
>>   	switch (sa->state) {
>>   	case SFC_ADAPTER_CONFIGURED:
>> diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
>> index e6809bb64..695580b22 100644
>> --- a/drivers/net/sfc/sfc_rx.c
>> +++ b/drivers/net/sfc/sfc_rx.c
>> @@ -618,7 +618,8 @@ struct sfc_dp_rx sfc_efx_rx = {
>>   	},
>>   	.features		= SFC_DP_RX_FEAT_INTR,
>>   	.dev_offload_capa	= DEV_RX_OFFLOAD_CHECKSUM,
>> -	.queue_offload_capa	= DEV_RX_OFFLOAD_SCATTER,
>> +	.queue_offload_capa	= DEV_RX_OFFLOAD_SCATTER |
>> +				  DEV_RX_OFFLOAD_RSS_HASH,
>
>Please, move to dev_offload_capa.
>
>>   	.qsize_up_rings		= sfc_efx_rx_qsize_up_rings,
>>   	.qcreate		= sfc_efx_rx_qcreate,
>>   	.qdestroy		= sfc_efx_rx_qdestroy,
>
>[snip]