Re:Re: [PATCH v1 1/3] examples/l3fwd: relax RSS requirement with option

[taozj888 <taozj888@163.com>]

[-- Attachment #1: Type: text/plain, Size: 4604 bytes --]

Hi Konstantin:


For your comments, please see my answers inline.
Thanks.


Trevor Tao

At 2023-10-09 16:43:01, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>Hi Trevor,
>
>> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS
>> by default, but some hw and/or virtual interface does not
>> support the RSS and offload mode presupposed, e.g., some
>> virtio interfaces in the cloud don't support
>> RSS and the error msg may like:
>> 
>> virtio_dev_configure(): RSS support requested but not supported by
>> the device
>> Port0 dev_configure = -95
>> 
>> So to enable the l3fwd running in that environment, the Rx mode requirement
>> can be relaxed to reflect the hardware feature reality here, and the l3fwd
>> can run smoothly then.
>> 
>> An option named "relax-rx-mode" is added to enable the relax action
>> here, and it's disabled by default.
>> 
>> Signed-off-by: Trevor Tao <taozj888@163.com>
>> ---
>>   examples/l3fwd/main.c | 19 +++++++++++++++++--
>>   1 file changed, 17 insertions(+), 2 deletions(-)
>> 
>> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
>> index 6063eb1399..2c8f528d00 100644
>> --- a/examples/l3fwd/main.c
>> +++ b/examples/l3fwd/main.c
>> @@ -73,6 +73,7 @@ static enum L3FWD_LOOKUP_MODE lookup_mode;
>>   static int numa_on = 1; /**< NUMA is enabled by default. */
>>   static int parse_ptype; /**< Parse packet type using rx callback, and */
>>   			/**< disabled by default */
>> +static int relax_rx_mode; /**< Relax RX mode is disabled by default */
>>   static int per_port_pool; /**< Use separate buffer pools per port; disabled */
>>   			  /**< by default */
>>   
>> @@ -678,6 +679,7 @@ static const char short_options[] =
>>   #define CMD_LINE_OPT_MAX_PKT_LEN "max-pkt-len"
>>   #define CMD_LINE_OPT_HASH_ENTRY_NUM "hash-entry-num"
>>   #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
>> +#define CMD_LINE_OPT_RELAX_RX_MODE "relax-rx-mode"
>>   #define CMD_LINE_OPT_PER_PORT_POOL "per-port-pool"
>>   #define CMD_LINE_OPT_MODE "mode"
>>   #define CMD_LINE_OPT_EVENTQ_SYNC "eventq-sched"
>> @@ -705,6 +707,7 @@ enum {
>>   	CMD_LINE_OPT_MAX_PKT_LEN_NUM,
>>   	CMD_LINE_OPT_HASH_ENTRY_NUM_NUM,
>>   	CMD_LINE_OPT_PARSE_PTYPE_NUM,
>> +	CMD_LINE_OPT_RELAX_RX_MODE_NUM,
>>   	CMD_LINE_OPT_RULE_IPV4_NUM,
>>   	CMD_LINE_OPT_RULE_IPV6_NUM,
>>   	CMD_LINE_OPT_ALG_NUM,
>> @@ -728,6 +731,7 @@ static const struct option lgopts[] = {
>>   	{CMD_LINE_OPT_MAX_PKT_LEN, 1, 0, CMD_LINE_OPT_MAX_PKT_LEN_NUM},
>>   	{CMD_LINE_OPT_HASH_ENTRY_NUM, 1, 0, CMD_LINE_OPT_HASH_ENTRY_NUM_NUM},
>>   	{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, CMD_LINE_OPT_PARSE_PTYPE_NUM},
>> +	{CMD_LINE_OPT_RELAX_RX_MODE, 0, 0, CMD_LINE_OPT_RELAX_RX_MODE_NUM},
>>   	{CMD_LINE_OPT_PER_PORT_POOL, 0, 0, CMD_LINE_OPT_PARSE_PER_PORT_POOL},
>>   	{CMD_LINE_OPT_MODE, 1, 0, CMD_LINE_OPT_MODE_NUM},
>>   	{CMD_LINE_OPT_EVENTQ_SYNC, 1, 0, CMD_LINE_OPT_EVENTQ_SYNC_NUM},
>> @@ -853,6 +857,11 @@ parse_args(int argc, char **argv)
>>   			parse_ptype = 1;
>>   			break;
>>   
>> +		case CMD_LINE_OPT_RELAX_RX_MODE_NUM:
>> +			printf("Relax rx mode is enabled\n");
>> +			relax_rx_mode = 1;
>> +			break;
>> +
>>   		case CMD_LINE_OPT_PARSE_PER_PORT_POOL:
>>   			printf("per port buffer pool is enabled\n");
>>   			per_port_pool = 1;
>> @@ -1257,8 +1266,14 @@ l3fwd_poll_resource_setup(void)
>>   		local_port_conf.rx_adv_conf.rss_conf.rss_hf &=
>>   			dev_info.flow_type_rss_offloads;
>>   
>> -		if (dev_info.max_rx_queues == 1)
>> -			local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
>> +		/* relax the rx rss requirement */
>> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
>> +			if (relax_rx_mode) {
>> +				printf("warning: modified the rx mq_mode to RTE_ETH_MQ_RX_NONE base on"
>> +					" device capability\n");
>> +				local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
>> +			}
>> +		}
>
>But that way we change current behavior - always use MQ_RX_NONE
>for devices with just one RX queue.
>Was it intended?
>Might be it should be:
>	if (dev_info.max_rx_queues == 1)
>		local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
>
>+	/* relax the rx rss requirement */
>+	if (relax_rx_mode &&
>+ 			!local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
>+		printf("...");
>+		local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
>+	}
>

>?
>OK, I will change it as what you gave here though I think there is no difference between the max_rx_queue is 1 and rss_hf is 0, both of them mean that RSS is not supported.
Thanks. 
>
>
>>   
>>   		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
>>   				port_conf.rx_adv_conf.rss_conf.rss_hf) {

[-- Attachment #2: Type: text/html, Size: 5731 bytes --]