Re: [dpdk-dev] [PATCH v6] app/testpmd: add forwarding mode to simulate a noisy neighbour

[Jens Freimann <jfreimann@redhat.com>]

On Thu, Sep 20, 2018 at 05:50:39PM +0100, Kevin Traynor wrote:
>On 09/18/2018 10:35 AM, Jens Freimann wrote:
>> This adds a new forwarding mode to testpmd to simulate
>> more realistic behavior of a guest machine engaged in receiving
>> and sending packets performing Virtual Network Function (VNF).
>>
>> The goal is to enable a simple way of measuring performance impact on
>> cache and memory footprint utilization from various VNF co-located on
>> the same host machine. For this it does:
>>
>> * Buffer packets in a FIFO:
>>
>> Create a fifo to buffer received packets. Once it flows over put
>> those packets into the actual tx queue. The fifo is created per tx
>> queue and its size can be set with the --noisy-tx-sw-buffer-flushtime
>> commandline parameter.
>>
>> A second commandline parameter is used to set a timeout in
>> milliseconds after which the fifo is flushed.
>>
>> --noisy-tx-sw-buffer-size [packet numbers]
>> Keep the mbuf in a FIFO and forward the over flooding packets from the
>> FIFO. This queue is per TX-queue (after all other packet processing).
>>
>> --noisy-tx-sw-buffer-flushtime [delay]
>> Flush the packet queue if no packets have been seen during
>> [delay]. As long as packets are seen, the timer is reset.
>>
>> Add several options to simulate route lookups (memory reads) in tables
>> that can be quite large, as well as route hit statistics update.
>> These options simulates the while stack traversal and
>> will trash the cache. Memory access is random.
>>
>> * simulate route lookups:
>>
>> Allocate a buffer and perform reads and writes on it as specified by
>> commandline options:
>>
>> --noisy-lkup-memory [size]
>> Size of the VNF internal memory (MB), in which the random
>> read/write will be done, allocated by rte_malloc (hugepages).
>>
>> --noisy-lkup-num-writes [num]
>> Number of random writes in memory per packet should be
>> performed, simulating hit-flags update. 64 bits per write,
>> all write in different cache lines.
>>
>> --noisy-lkup-num-reads [num]
>> Number of random reads in memory per packet should be
>> performed, simulating FIB/table lookups. 64 bits per read,
>> all write in different cache lines.
>>
>> --noisy-lkup-num-reads-writes [num]
>> Number of random reads and writes in memory per packet should
>> be performed, simulating stats update. 64 bits per read-write, all
>> reads and writes in different cache lines.
>>
>> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>> ---
>
>Hi Jens, thanks for the new version. A small few remaining comments below,
>
>Kevin.
>
><snip>
>
>> +
>> +static void
>> +noisy_fwd_begin(portid_t pi)
>> +{
>> +	struct noisy_config *n;
>> +	char name[NOISY_STRSIZE];
>> +
>> +	noisy_cfg[pi] = rte_zmalloc("testpmd noisy fifo and timers",
>> +				sizeof(struct noisy_config),
>> +				RTE_CACHE_LINE_SIZE);
>> +	if (noisy_cfg == NULL) {
>
>Looks like it should be 'if (noisy_cfg[pi] == NULL)'

yep, fixed.

>> +		rte_exit(EXIT_FAILURE,
>> +			 "rte_zmalloc(%d) struct noisy_config) \
>> +			 failed\n", (int) pi);
>> +	}
>> +	n = noisy_cfg[pi];
>> +	n->do_buffering = noisy_tx_sw_bufsz > 0;
>> +	n->do_sim = noisy_lkup_num_writes + noisy_lkup_num_reads +
>> +		    noisy_lkup_num_reads_writes;
>> +	n->do_flush = noisy_tx_sw_buf_flush_time > 0;
>> +
>> +	if (n->do_buffering) {
>> +		snprintf(name, NOISY_STRSIZE, NOISY_RING, pi);
>> +		n->f = rte_ring_create(name, noisy_tx_sw_bufsz,
>> +				rte_socket_id(), 0);
>> +		if (!n->f)
>> +			rte_exit(EXIT_FAILURE,
>> +				 "rte_ring_create(%d), size %d) \
>> +				 failed\n", (int) pi,
>> +				 noisy_tx_sw_bufsz);
>> +	}
>> +	if (noisy_lkup_mem_sz > 0) {
>> +		n->vnf_mem = (char *) rte_zmalloc("vnf sim memory",
>> +				 noisy_lkup_mem_sz * 1024 * 1024,
>> +				 RTE_CACHE_LINE_SIZE);
>> +		if (!n->vnf_mem)
>> +			rte_exit(EXIT_FAILURE,
>> +				 "rte_zmalloc(%" PRIu64 ") for vnf \
>> +				 memory) failed\n", noisy_lkup_mem_sz);
>> +	} else if (n->do_sim) {
>> +		rte_exit(EXIT_FAILURE, "--noisy-lkup-memory-size \
>> +			 must be > 0\n");
>> +	}
>> +}
>> +
>> +struct fwd_engine noisy_vnf_engine = {
>> +	.fwd_mode_name  = "noisy",
>> +	.port_fwd_begin = noisy_fwd_begin,
>> +	.port_fwd_end   = noisy_fwd_end,
>> +	.packet_fwd     = pkt_burst_noisy_vnf,
>> +};
>> +
>
>new blank line at EOF.
>+
>warning: 1 line adds whitespace errors.

hmpf

>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>> index 9220e1c1b..3231b0c51 100644
>> --- a/app/test-pmd/parameters.c
>> +++ b/app/test-pmd/parameters.c
>> @@ -625,6 +625,12 @@ launch_args_parse(int argc, char** argv)
>>  		{ "vxlan-gpe-port",		1, 0, 0 },
>>  		{ "mlockall",			0, 0, 0 },
>>  		{ "no-mlockall",		0, 0, 0 },
>> +		{ "noisy-tx-sw-buffer-size",	1, 0, 0 },
>> +		{ "noisy-tx-sw-buffer-flushtime",1, 0, 0 },
>> +		{ "noisy-lkup-memory",		1, 0, 0 },
>> +		{ "noisy-lkup-num-writes",	1, 0, 0 },
>> +		{ "noisy-lkup-num-reads",	1, 0, 0 },
>> +		{ "noisy-lkup-num-reads-writes",1, 0, 0 },
>>  		{ 0, 0, 0, 0 },
>>  	};
>>
>> @@ -1145,6 +1151,60 @@ launch_args_parse(int argc, char** argv)
>>  				do_mlockall = 1;
>>  			if (!strcmp(lgopts[opt_idx].name, "no-mlockall"))
>>  				do_mlockall = 0;
>> +			if (!strcmp(lgopts[opt_idx].name,
>> +				    "noisy-tx-sw-buffer-size")) {
>> +				n = atoi(optarg);
>> +				if (n >= 0)
>> +					noisy_tx_sw_bufsz = n;
>> +				else
>> +					rte_exit(EXIT_FAILURE,
>> +						"noisy-tx-sw-buffer-size must be >= 0\n");
>> +			}
>> +			if (!strcmp(lgopts[opt_idx].name,
>> +				    "noisy-tx-sw-buffer-flushtime")) {
>> +				n = atoi(optarg);
>> +				if (n >= 0)
>> +					noisy_tx_sw_buf_flush_time = n;
>> +				else
>> +					rte_exit(EXIT_FAILURE,
>> +						 "noisy-tx-sw-buffer-flushtime must be >= 0\n");
>> +			}
>> +			if (!strcmp(lgopts[opt_idx].name,
>> +				    "noisy-lkup-memory")) {
>> +				n = atoi(optarg);
>> +				if (n > 0)
>
>I thought this and below ones would also be '>=' also?

I wasn't sure if it's a good idea or if I should choose a different
default... and then forgot about it. sorry. I checked that it's safe
to allow 0, will keep the defaults at 0 and allow it to be set here.

Thanks for the review Kevin!

regards,
Jens