DPDK patches and discussions
 help / color / Atom feed
From: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
To: Andrzej Ostruszka <amo@semihalf.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [dpdk-dev v2] [PATCH] examples/l2fwd-event: add option to configure port pairs
Date: Tue, 31 Mar 2020 12:17:52 +0000
Message-ID: <CY4PR1801MB1863190FFDFC69DD6083730FDEC80@CY4PR1801MB1863.namprd18.prod.outlook.com> (raw)
In-Reply-To: <eeb16761-fbd4-25bf-6b22-d1c891fdbc25@semihalf.com>

>> ---
>>  v2 Changes:
>>  - Fix minor formatting error.
>>  - Change uint8_t to bool.
>[...]
>> @@ -99,6 +103,69 @@ l2fwd_event_parse_eventq_sched(const char
>*optarg,
>>  		rsrc->sched_type = RTE_SCHED_TYPE_PARALLEL;
>>  }
>>
>> +static int
>> +l2fwd_parse_port_pair_config(const char *q_arg, struct
>l2fwd_resources *rsrc)
>> +{
>> +	enum fieldnames {
>> +		FLD_PORT1 = 0,
>> +		FLD_PORT2,
>> +		_NUM_FLD
>> +	};
>> +	const char *p, *p0 = q_arg;
>> +	uint16_t int_fld[_NUM_FLD];
>> +	char *str_fld[_NUM_FLD];
>> +	uint16_t port_pair = 0;
>> +	unsigned int size;
>> +	char s[256];
>> +	char *end;
>> +	int i;
>> +
>> +	while ((p = strchr(p0, '(')) != NULL) {
>> +		++p;
>> +		p0 = strchr(p, ')');
>> +		if (p0 == NULL)
>> +			return -1;
>> +
>> +		size = p0 - p;
>> +		if (size >= sizeof(s))
>> +			return -1;
>> +
>> +		snprintf(s, sizeof(s), "%.*s", size, p);
>
>This is a bit peculiar form of memcpy - you want no more than sizeof(s)
>copied but that you checked above so here simple memcpy should be
>enough.

This is a remnant of l3fwd --config options parsing, I will change it to memcpy in 
next version.

>
>> +		if (rte_strsplit(s, sizeof(s), str_fld,
>> +					_NUM_FLD, ',') != _NUM_FLD)
>> +			return -1;
>> +
>> +		for (i = 0; i < _NUM_FLD; i++) {
>> +			errno = 0;
>> +			int_fld[i] = strtoul(str_fld[i], &end, 0);
>> +			if (errno != 0 || end == str_fld[i] || int_fld[i] >
>255)
>
>Replace 255 with check on ">= RTE_MAX_ETHPORTS".	

Will fix in next version.

>
>> +				return -1;
>> +		}
>> +
>> +		if (port_pair >= RTE_MAX_ETHPORTS / 2) {
>> +			printf("exceeded max number of port pair
>params: Current %d Max = %d\n",
>> +			       port_pair, RTE_MAX_ETHPORTS / 2);
>> +			return -1;
>> +		}
>> +
>> +		if ((rsrc->dst_ports[int_fld[FLD_PORT1]] !=
>UINT32_MAX) ||
>> +			(rsrc->dst_ports[int_fld[FLD_PORT2]] !=
>UINT32_MAX)) {
>> +			printf("Duplicate port pair (%d,%d) config\n",
>> +					int_fld[FLD_PORT1],
>int_fld[FLD_PORT2]);
>> +			return -1;
>> +		}
>> +
>> +		rsrc->dst_ports[int_fld[FLD_PORT1]] =
>int_fld[FLD_PORT2];
>> +		rsrc->dst_ports[int_fld[FLD_PORT2]] =
>int_fld[FLD_PORT1];
>> +
>> +		port_pair++;
>> +	}
>> +
>> +	rsrc->port_pairs = true;
>> +
>> +	return 0;
>> +}
>> +
>[...]
>> @@ -209,6 +293,51 @@ l2fwd_event_parse_args(int argc, char
>**argv,
>>  	return ret;
>>  }
>>
>> +/*
>> + * Check port pair config with enabled port mask,
>> + * and for valid port pair combinations.
>> + */
>> +static int
>> +check_port_pair_config(struct l2fwd_resources *rsrc)
>> +{
>> +	uint32_t port_pair_mask = 0;
>> +	uint32_t portid;
>> +	uint16_t index;
>> +
>> +	for (index = 0; index < rte_eth_dev_count_avail(); index++) {
>> +		if ((rsrc->enabled_port_mask & (1 << index)) == 0)
>> +			continue;
>> +
>> +		portid = rsrc->dst_ports[index];
>> +		if (portid == UINT32_MAX) {
>> +			printf("port %u is enabled in but no valid port
>pair\n",
>> +			       index);
>> +			return -1;
>> +		}
>> +
>> +		if (!rte_eth_dev_is_valid_port(index)) {
>> +			printf("port %u is not valid\n", index);
>> +			return -1;
>> +		}
>> +
>> +		if (!rte_eth_dev_is_valid_port(portid)) {
>> +			printf("port %u is not valid\n", portid);
>> +			return -1;
>> +		}
>> +
>> +		if (port_pair_mask & (1 << portid) &&
>> +				rsrc->dst_ports[portid] != index) {
>> +			printf("port %u is used in other port pairs\n",
>portid);
>> +			return -1;
>> +		}
>> +
>> +		port_pair_mask |= (1 << portid);
>> +		port_pair_mask |= (1 << index);
>> +	}
>
>In the above loop you are doing checks twice.  Suppose you have pair
>(2,3) and you go by index from 0 (like you do) and reach point i=2.
>Then you check i=2 and p=3, then on next iteration you do the same
>checks (this time i=3,p=2).  I guess simple fix would be to skip loop
>iteration both on not enabled (like you do) and on check if the port was
>already checked (test bit in port_pair_mask).

Ack, will fix in v3.

>
>With regards
>Andrzej Ostruszka

Thanks,
Pavan.

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24 12:34 [dpdk-dev] [PATCH] example/l2fwd-event: add option to configure forwarding info pbhagavatula
2020-03-24 12:34 ` [dpdk-dev] [PATCH] examples/l2fwd-event: add option to configure port pairs pbhagavatula
2020-03-25 10:40   ` Sunil Kumar Kori
2020-03-26  6:37     ` Pavan Nikhilesh Bhagavatula
2020-03-26  6:42   ` [dpdk-dev] [dpdk-dev v2] " pbhagavatula
2020-03-31 10:23     ` Andrzej Ostruszka
2020-03-31 12:17       ` Pavan Nikhilesh Bhagavatula [this message]
2020-03-31 12:53     ` [dpdk-dev] [PATCH v3] " pbhagavatula
2020-03-31 17:07       ` Andrzej Ostruszka
2020-04-04 16:10         ` Jerin Jacob

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CY4PR1801MB1863190FFDFC69DD6083730FDEC80@CY4PR1801MB1863.namprd18.prod.outlook.com \
    --to=pbhagavatula@marvell.com \
    --cc=amo@semihalf.com \
    --cc=dev@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox