Re: [dpdk-dev] [PATCH v3 0/2] User-space ethtool sample application

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Remy

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton
> Sent: Wednesday, October 28, 2015 11:12 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 0/2] User-space ethtool sample application
> 
> Further enhancements to the userspace ethtool implementation that was
> submitted in 2.1 and packaged as a self-contained sample application.
> Implements an rte_ethtool shim layer based on rte_ethdev API, along
> with a command prompt driven demonstration application.

Looks good in general, just few small things, see below.
Thanks
Konstantin


> 
> This patchset depends on:
> * http://dpdk.org/dev/patchwork/patch/6563/
> * http://dpdk.org/dev/patchwork/patch/7340/
> * http://dpdk.org/dev/patchwork/patch/8070/
> * http://dpdk.org/dev/patchwork/patch/8067/
> * http://dpdk.org/dev/patchwork/patch/8075/
> * http://dpdk.org/dev/patchwork/patch/8074/
> * http://dpdk.org/dev/patchwork/patch/8072/
> * http://dpdk.org/dev/patchwork/patch/8071/
> * http://dpdk.org/dev/patchwork/patch/8073/
> * http://dpdk.org/dev/patchwork/patch/8068/
> * http://dpdk.org/dev/patchwork/patch/8069/

I think it actually depends only on these two:
http://dpdk.org/dev/patchwork/patch/6563/
http://dpdk.org/dev/patchwork/patch/8070/

1) From [PATCH v3 1/2] example: add user-space ethtool sample application

+int main(int argc, char **argv)
+{
+	int cnt_args_parsed;
+	uint32_t idx_port;
+	uint32_t id_core;
+	uint32_t cnt_ports;
+	struct app_port *ptr_port;
+
+	/* Init runtime enviornment */
+	cnt_args_parsed = rte_eal_init(argc, argv);
+	if (cnt_args_parsed < 0)
+		rte_exit(EXIT_FAILURE, "rte_eal_init(): Failed");
+
+	cnt_ports = rte_eth_dev_count();
+	printf("Eth NICs: %i\n", cnt_ports);
+	if (cnt_ports > MAX_PORTS) {
+		printf("Info: Using only %i of %i ports\n",
+			cnt_ports, MAX_PORTS
+			);
+		cnt_ports = MAX_PORTS;
+	}
+
+	setup_ports(&app_cfg, cnt_ports);
+
+	id_core = rte_lcore_id();
+	for (idx_port = 0; idx_port < cnt_ports; idx_port++) {
+		id_core = rte_get_next_lcore(id_core, 1, 0);
+		if (id_core == RTE_MAX_LCORE) {
+			printf("Warning: More ports than cores. "
+				"Some ports will not be active.\n");
+			break;
+		}

Master core is not always the first active core.
User can select a master lcore with '--master-lcore X' command-line option.
Another thing - why do you need a separate lcore for each port?
It means that if user would like to test X different ports he/she would need X+1 lcores to run the app.
I understand that you are trying to keep the app as simple as possible, but why not visa-versa then?
Make all ports managed by one slave lcore?
It might slowdown things as number of ports grow, but from other side this app is
mainly for functional testing of ethtool shim layer, so I think it is not a problem.

+		ptr_port = &app_cfg.ports[idx_port];
+		rte_eal_remote_launch(slave_main, ptr_port, id_core);
+	}
+
+	ethapp_main();
+
+	for (idx_port = 0; idx_port < cnt_ports; idx_port++)
+		app_cfg.ports[idx_port].exit_now = 1;
+	RTE_LCORE_FOREACH_SLAVE(id_core) {
+		if (rte_eal_wait_lcore(id_core) < 0)
+			return -1;
+	}
+
+	return 0;
+}

2) From: [PATCH v3 2/2] doc: add user-space ethtool sample app guide

It looks like a guide for keep-alive not ehttool :)