[dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Raslan Darawsheh]

Added hotplug in testpmd, to be able to test hotplug function
in the PMD's.

Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
---
 app/test-pmd/cmdline.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.c | 18 ++++++++++++++++++
 app/test-pmd/testpmd.h |  1 +
 3 files changed, 63 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index cd8c358..b32a368 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"port config (port_id|all) l2-tunnel E-tag"
 			" (enable|disable)\n"
 			"    Enable/disable the E-tag support.\n\n"
+
+			" device remove (device)\n"
+			"    Remove a device"
 		);
 	}
 
@@ -1125,6 +1128,46 @@ cmdline_parse_inst_t cmd_operate_detach_port = {
 	},
 };
 
+/* *** Remove a specified device *** */
+struct cmd_operate_device_remove_result {
+	cmdline_fixed_string_t device;
+	cmdline_fixed_string_t keyword;
+	cmdline_fixed_string_t identifier;
+};
+
+static void cmd_operate_device_remove_parsed(void *parsed_result,
+				   __attribute__((unused)) struct cmdline *cl,
+				   __attribute__((unused)) void *data)
+{
+	struct cmd_operate_device_remove_result *res = parsed_result;
+	if (!strcmp(res->keyword, "remove"))
+		device_remove((char *) res->identifier);
+	else
+		printf("Unknown parameter\n");
+}
+
+cmdline_parse_token_string_t cmd_operate_device_remove_device =
+	TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result,
+			device, "device");
+cmdline_parse_token_string_t cmd_operate_device_remove_keyword =
+	TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result,
+			keyword, "remove");
+cmdline_parse_token_string_t cmd_operate_device_remove_identifier =
+	TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result,
+			identifier, NULL);
+
+cmdline_parse_inst_t cmd_operate_device_remove = {
+	.f = cmd_operate_device_remove_parsed,
+	.data = NULL,
+	.help_str = "device remove <device>: (device)",
+	.tokens = {
+		(void *)&cmd_operate_device_remove_device,
+		(void *)&cmd_operate_device_remove_keyword,
+		(void *)&cmd_operate_device_remove_identifier,
+		NULL,
+	},
+};
+
 /* *** configure speed for all ports *** */
 struct cmd_config_speed_all {
 	cmdline_fixed_string_t port;
@@ -14276,6 +14319,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_operate_specific_port,
 	(cmdline_parse_inst_t *)&cmd_operate_attach_port,
 	(cmdline_parse_inst_t *)&cmd_operate_detach_port,
+	(cmdline_parse_inst_t *)&cmd_operate_device_remove,
 	(cmdline_parse_inst_t *)&cmd_config_speed_all,
 	(cmdline_parse_inst_t *)&cmd_config_speed_specific,
 	(cmdline_parse_inst_t *)&cmd_config_rx_tx,
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 7d40139..a2e8526 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1742,6 +1742,24 @@ detach_port(uint8_t port_id)
 }
 
 void
+device_remove(char *device) {
+	struct rte_devargs devargs;
+
+	if (device == NULL) {
+		printf("Invalid parameters are specified\n");
+		return;
+	}
+
+	rte_eal_devargs_parse(device, &devargs);
+
+	if (rte_eal_hotplug_remove(devargs.bus->name, devargs.name)) {
+		printf("Fail to remove device\n");
+		return;
+	}
+	printf("Device removed successfully\n");
+}
+
+void
 pmd_test_exit(void)
 {
 	portid_t pt_id;
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index c9d7739..0780f55 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -612,6 +612,7 @@ void stop_port(portid_t pid);
 void close_port(portid_t pid);
 void attach_port(char *identifier);
 void detach_port(uint8_t port_id);
+void device_remove(char *device);
 int all_ports_stopped(void);
 int port_is_started(portid_t port_id);
 void pmd_test_exit(void);
-- 
2.7.4

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Gaëtan Rivet]

Hello Raslan,

On Wed, Aug 23, 2017 at 05:37:04PM +0300, Raslan Darawsheh wrote:
> Added hotplug in testpmd, to be able to test hotplug function
> in the PMD's.
> 
> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> ---
>  app/test-pmd/cmdline.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  app/test-pmd/testpmd.c | 18 ++++++++++++++++++
>  app/test-pmd/testpmd.h |  1 +
>  3 files changed, 63 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index cd8c358..b32a368 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>  			"port config (port_id|all) l2-tunnel E-tag"
>  			" (enable|disable)\n"
>  			"    Enable/disable the E-tag support.\n\n"
> +
> +			" device remove (device)\n"
> +			"    Remove a device"

I think it should still be a part of the "port" command set (port
attach|detach|stop|close, etc).

This would probably be easier to understand for users.

>  		);
>  	}
>  
> @@ -1125,6 +1128,46 @@ cmdline_parse_inst_t cmd_operate_detach_port = {
>  	},
>  };
>  
> +/* *** Remove a specified device *** */
> +struct cmd_operate_device_remove_result {
> +	cmdline_fixed_string_t device;
> +	cmdline_fixed_string_t keyword;
> +	cmdline_fixed_string_t identifier;
> +};
> +
> +static void cmd_operate_device_remove_parsed(void *parsed_result,
> +				   __attribute__((unused)) struct cmdline *cl,
> +				   __attribute__((unused)) void *data)
> +{
> +	struct cmd_operate_device_remove_result *res = parsed_result;
> +	if (!strcmp(res->keyword, "remove"))
> +		device_remove((char *) res->identifier);
> +	else
> +		printf("Unknown parameter\n");
> +}
> +
> +cmdline_parse_token_string_t cmd_operate_device_remove_device =
> +	TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result,
> +			device, "device");
> +cmdline_parse_token_string_t cmd_operate_device_remove_keyword =
> +	TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result,
> +			keyword, "remove");
> +cmdline_parse_token_string_t cmd_operate_device_remove_identifier =
> +	TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result,
> +			identifier, NULL);
> +
> +cmdline_parse_inst_t cmd_operate_device_remove = {
> +	.f = cmd_operate_device_remove_parsed,
> +	.data = NULL,
> +	.help_str = "device remove <device>: (device)",
> +	.tokens = {
> +		(void *)&cmd_operate_device_remove_device,
> +		(void *)&cmd_operate_device_remove_keyword,
> +		(void *)&cmd_operate_device_remove_identifier,
> +		NULL,
> +	},
> +};
> +

Continuing on using the

port ...

format, then the port_id should allow to remove it instead of the device
identifier.
Using the device identifier will complexify your implementation.

>  /* *** configure speed for all ports *** */
>  struct cmd_config_speed_all {
>  	cmdline_fixed_string_t port;
> @@ -14276,6 +14319,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_operate_specific_port,
>  	(cmdline_parse_inst_t *)&cmd_operate_attach_port,
>  	(cmdline_parse_inst_t *)&cmd_operate_detach_port,
> +	(cmdline_parse_inst_t *)&cmd_operate_device_remove,
>  	(cmdline_parse_inst_t *)&cmd_config_speed_all,
>  	(cmdline_parse_inst_t *)&cmd_config_speed_specific,
>  	(cmdline_parse_inst_t *)&cmd_config_rx_tx,
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 7d40139..a2e8526 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1742,6 +1742,24 @@ detach_port(uint8_t port_id)
>  }
>  
>  void
> +device_remove(char *device) {
> +	struct rte_devargs devargs;
> +
> +	if (device == NULL) {
> +		printf("Invalid parameters are specified\n");
> +		return;
> +	}

I don't think those checks are necessary. This function should not be
called with invalid parameters, and you will be the only one calling it.

> +
> +	rte_eal_devargs_parse(device, &devargs);

If you used a port_id, you would not have to parse the device string.
You could do something like this:

	struct rte_eth_dev *eth_dev;
	struct rte_bus *bus;

	eth_dev = &rte_eth_devices[port_id];
	bus = rte_bus_find_by_device(eth_dev->device);
> +
> +	if (rte_eal_hotplug_remove(devargs.bus->name, devargs.name)) {

  +	if (rte_eal_hotplug_remove(bus->name, eth_dev->device->name)) {

As a side note, I don't see why we need to use those names in the
function parameters.
We could simply use the rte_device handle available. This handle should
propose all necessary infos.
It might be a future API change.

> +		printf("Fail to remove device\n");
> +		return;
> +	}
> +	printf("Device removed successfully\n");
> +}
> +
> +void
>  pmd_test_exit(void)
>  {
>  	portid_t pt_id;
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index c9d7739..0780f55 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -612,6 +612,7 @@ void stop_port(portid_t pid);
>  void close_port(portid_t pid);
>  void attach_port(char *identifier);
>  void detach_port(uint8_t port_id);
> +void device_remove(char *device);

void remove_port(uint8_t port_id);

>  int all_ports_stopped(void);
>  int port_is_started(portid_t port_id);
>  void pmd_test_exit(void);
> -- 
> 2.7.4
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Thomas Monjalon]

23/08/2017 17:09, Gaëtan Rivet:
> Hello Raslan,
> 
> On Wed, Aug 23, 2017 at 05:37:04PM +0300, Raslan Darawsheh wrote:
> > Added hotplug in testpmd, to be able to test hotplug function
> > in the PMD's.
> > 
> > Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
[...]
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
> >  			"port config (port_id|all) l2-tunnel E-tag"
> >  			" (enable|disable)\n"
> >  			"    Enable/disable the E-tag support.\n\n"
> > +
> > +			" device remove (device)\n"
> > +			"    Remove a device"
> 
> I think it should still be a part of the "port" command set (port
> attach|detach|stop|close, etc).

I tend to disagree.
As far as I know, we use port for ethdev or cryptodev.
Here we want to deal with EAL rte_device.

> This would probably be easier to understand for users.

[...]
> Continuing on using the port ...
> format, then the port_id should allow to remove it instead of the device
> identifier.
> Using the device identifier will complexify your implementation.
[...]
> 	eth_dev = &rte_eth_devices[port_id];
> 	bus = rte_bus_find_by_device(eth_dev->device);

Note that we are going to remove eth_dev->device which implies eth_dev
but maybe also more device interfaces for the same HW.
That's why I think we need to distinguish port and device somehow.

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Gaëtan Rivet]

On Wed, Aug 23, 2017 at 06:18:34PM +0200, Thomas Monjalon wrote:
> 23/08/2017 17:09, Gaëtan Rivet:
> > Hello Raslan,
> > 
> > On Wed, Aug 23, 2017 at 05:37:04PM +0300, Raslan Darawsheh wrote:
> > > Added hotplug in testpmd, to be able to test hotplug function
> > > in the PMD's.
> > > 
> > > Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> [...]
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
> > >  			"port config (port_id|all) l2-tunnel E-tag"
> > >  			" (enable|disable)\n"
> > >  			"    Enable/disable the E-tag support.\n\n"
> > > +
> > > +			" device remove (device)\n"
> > > +			"    Remove a device"
> > 
> > I think it should still be a part of the "port" command set (port
> > attach|detach|stop|close, etc).
> 
> I tend to disagree.
> As far as I know, we use port for ethdev or cryptodev.
> Here we want to deal with EAL rte_device.
> 

I see, that makes sense.
I will redo the review with that in mind.

> > This would probably be easier to understand for users.
> 
> [...]
> > Continuing on using the port ...
> > format, then the port_id should allow to remove it instead of the device
> > identifier.
> > Using the device identifier will complexify your implementation.
> [...]
> > 	eth_dev = &rte_eth_devices[port_id];
> > 	bus = rte_bus_find_by_device(eth_dev->device);
> 
> Note that we are going to remove eth_dev->device which implies eth_dev
> but maybe also more device interfaces for the same HW.
> That's why I think we need to distinguish port and device somehow.

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Thomas Monjalon]

25/08/2017 09:53, Gaëtan Rivet:
> On Wed, Aug 23, 2017 at 06:18:34PM +0200, Thomas Monjalon wrote:
> > 23/08/2017 17:09, Gaëtan Rivet:
> > > Hello Raslan,
> > > 
> > > On Wed, Aug 23, 2017 at 05:37:04PM +0300, Raslan Darawsheh wrote:
> > > > Added hotplug in testpmd, to be able to test hotplug function
> > > > in the PMD's.
> > > > 
> > > > Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> > [...]
> > > > --- a/app/test-pmd/cmdline.c
> > > > +++ b/app/test-pmd/cmdline.c
> > > > @@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
> > > >  			"port config (port_id|all) l2-tunnel E-tag"
> > > >  			" (enable|disable)\n"
> > > >  			"    Enable/disable the E-tag support.\n\n"
> > > > +
> > > > +			" device remove (device)\n"
> > > > +			"    Remove a device"
> > > 
> > > I think it should still be a part of the "port" command set (port
> > > attach|detach|stop|close, etc).
> > 
> > I tend to disagree.
> > As far as I know, we use port for ethdev or cryptodev.
> > Here we want to deal with EAL rte_device.
> > 
> 
> I see, that makes sense.
> I will redo the review with that in mind.

One option is to reword the command to show that it removes
the device associated to the specified port.

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Gaëtan Rivet]

Hi Raslan,

Redoing the review with the remarks from Thomas in mind.

On Wed, Aug 23, 2017 at 05:37:04PM +0300, Raslan Darawsheh wrote:
> Added hotplug in testpmd, to be able to test hotplug function
> in the PMD's.
> 
> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> ---
>  app/test-pmd/cmdline.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  app/test-pmd/testpmd.c | 18 ++++++++++++++++++
>  app/test-pmd/testpmd.h |  1 +
>  3 files changed, 63 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index cd8c358..b32a368 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>  			"port config (port_id|all) l2-tunnel E-tag"
>  			" (enable|disable)\n"
>  			"    Enable/disable the E-tag support.\n\n"
> +
> +			" device remove (device)\n"

I think (device) might be hard to understand for a user.
Maybe (device name)?

> +			"    Remove a device"

Here it should be:

+			"    Remove a device.\n\n"

>  		);
>  	}
>  
> @@ -1125,6 +1128,46 @@ cmdline_parse_inst_t cmd_operate_detach_port = {
>  	},
>  };
>  
> +/* *** Remove a specified device *** */
> +struct cmd_operate_device_remove_result {
> +	cmdline_fixed_string_t device;
> +	cmdline_fixed_string_t keyword;
> +	cmdline_fixed_string_t identifier;
> +};
> +
> +static void cmd_operate_device_remove_parsed(void *parsed_result,
> +				   __attribute__((unused)) struct cmdline *cl,
> +				   __attribute__((unused)) void *data)
> +{
> +	struct cmd_operate_device_remove_result *res = parsed_result;
> +	if (!strcmp(res->keyword, "remove"))
> +		device_remove((char *) res->identifier);
> +	else
> +		printf("Unknown parameter\n");
> +}
> +
> +cmdline_parse_token_string_t cmd_operate_device_remove_device =
> +	TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result,
> +			device, "device");
> +cmdline_parse_token_string_t cmd_operate_device_remove_keyword =
> +	TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result,
> +			keyword, "remove");
> +cmdline_parse_token_string_t cmd_operate_device_remove_identifier =
> +	TOKEN_STRING_INITIALIZER(struct cmd_operate_device_remove_result,
> +			identifier, NULL);
> +
> +cmdline_parse_inst_t cmd_operate_device_remove = {
> +	.f = cmd_operate_device_remove_parsed,
> +	.data = NULL,
> +	.help_str = "device remove <device>: (device)",
> +	.tokens = {
> +		(void *)&cmd_operate_device_remove_device,
> +		(void *)&cmd_operate_device_remove_keyword,
> +		(void *)&cmd_operate_device_remove_identifier,

I know you have to follow the conventions set for this file and these
names are correct. But I have to say: they are horrible.

> +		NULL,
> +	},
> +};
> +
>  /* *** configure speed for all ports *** */
>  struct cmd_config_speed_all {
>  	cmdline_fixed_string_t port;
> @@ -14276,6 +14319,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_operate_specific_port,
>  	(cmdline_parse_inst_t *)&cmd_operate_attach_port,
>  	(cmdline_parse_inst_t *)&cmd_operate_detach_port,
> +	(cmdline_parse_inst_t *)&cmd_operate_device_remove,
>  	(cmdline_parse_inst_t *)&cmd_config_speed_all,
>  	(cmdline_parse_inst_t *)&cmd_config_speed_specific,
>  	(cmdline_parse_inst_t *)&cmd_config_rx_tx,
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 7d40139..a2e8526 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1742,6 +1742,24 @@ detach_port(uint8_t port_id)
>  }
>  
>  void
> +device_remove(char *device) {

device is not descriptive enough. I think "devname" would be better.

> +	struct rte_devargs devargs;
> +
> +	if (device == NULL) {
> +		printf("Invalid parameters are specified\n");
> +		return;
> +	}

No need to check for this. If device is NULL, something went very wrong.
Having a hard segfault in this case will stop anyone in their tracks and
force the bug to be investigated.

And it has less LOCs.

> +
> +	rte_eal_devargs_parse(device, &devargs);
> +

After some thinking, I think using the devargs_parse utility is the
simplest and will allow the command to follow future evolutions without
having to change anything here.

> +	if (rte_eal_hotplug_remove(devargs.bus->name, devargs.name)) {
> +		printf("Fail to remove device\n");
> +		return;
> +	}
> +	printf("Device removed successfully\n");
> +}
> +
> +void
>  pmd_test_exit(void)
>  {
>  	portid_t pt_id;
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index c9d7739..0780f55 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -612,6 +612,7 @@ void stop_port(portid_t pid);
>  void close_port(portid_t pid);
>  void attach_port(char *identifier);
>  void detach_port(uint8_t port_id);
> +void device_remove(char *device);
>  int all_ports_stopped(void);
>  int port_is_started(portid_t port_id);
>  void pmd_test_exit(void);
> -- 
> 2.7.4
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Ferruh Yigit]

On 8/28/2017 10:55 AM, Gaëtan Rivet wrote:
> Hi Raslan,
> 
> Redoing the review with the remarks from Thomas in mind.
> 
> On Wed, Aug 23, 2017 at 05:37:04PM +0300, Raslan Darawsheh wrote:
>> Added hotplug in testpmd, to be able to test hotplug function
>> in the PMD's.
>>
>> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
>> ---
>>  app/test-pmd/cmdline.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  app/test-pmd/testpmd.c | 18 ++++++++++++++++++
>>  app/test-pmd/testpmd.h |  1 +
>>  3 files changed, 63 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index cd8c358..b32a368 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>>  			"port config (port_id|all) l2-tunnel E-tag"
>>  			" (enable|disable)\n"
>>  			"    Enable/disable the E-tag support.\n\n"
>> +
>> +			" device remove (device)\n"
> 
> I think (device) might be hard to understand for a user.
> Maybe (device name)?

I am suspicious on adding new root level command to testpmd, it is
getting bigger, each command looks OK on its own context, but as a whole
app getting more confusing [1].

Since dealing with device is kind of new, it can be OK to create new
command tree, but there are already hotplug commands per port:
"port attach #PCI|#VDEV_NAME"
"port detach #P"

perhaps it can be good to keep "attach", "detach" keywords for device to
be consistent?

"device attach #name"
"device detach #name"

Also a show equivalent can be added to work in device level:
"show device info"


[1]
http://dpdk.org/ml/archives/dev/2017-August/072764.html

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Thomas Monjalon]

28/08/2017 12:30, Ferruh Yigit:
> On 8/28/2017 10:55 AM, Gaëtan Rivet wrote:
> > Hi Raslan,
> > 
> > Redoing the review with the remarks from Thomas in mind.
> > 
> > On Wed, Aug 23, 2017 at 05:37:04PM +0300, Raslan Darawsheh wrote:
> >> Added hotplug in testpmd, to be able to test hotplug function
> >> in the PMD's.
> >>
> >> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> >> ---
> >>  app/test-pmd/cmdline.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >>  app/test-pmd/testpmd.c | 18 ++++++++++++++++++
> >>  app/test-pmd/testpmd.h |  1 +
> >>  3 files changed, 63 insertions(+)
> >>
> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> >> index cd8c358..b32a368 100644
> >> --- a/app/test-pmd/cmdline.c
> >> +++ b/app/test-pmd/cmdline.c
> >> @@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
> >>  			"port config (port_id|all) l2-tunnel E-tag"
> >>  			" (enable|disable)\n"
> >>  			"    Enable/disable the E-tag support.\n\n"
> >> +
> >> +			" device remove (device)\n"
> > 
> > I think (device) might be hard to understand for a user.
> > Maybe (device name)?
> 
> I am suspicious on adding new root level command to testpmd, it is
> getting bigger, each command looks OK on its own context, but as a whole
> app getting more confusing [1].

We can keep the root level "port" if we make clear that the impact
is beyond the port.

> Since dealing with device is kind of new, it can be OK to create new
> command tree, but there are already hotplug commands per port:
> "port attach #PCI|#VDEV_NAME"
> "port detach #P"
> 
> perhaps it can be good to keep "attach", "detach" keywords for device to
> be consistent?

Not sure.
Anyway ethdev attach and detach should be deprecated.
We are moving to a more correct design where hotplug is done at EAL level.

> "device attach #name"
> "device detach #name"
> 
> Also a show equivalent can be added to work in device level:
> "show device info"
> 
> 
> [1]
> http://dpdk.org/ml/archives/dev/2017-August/072764.html

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Gaëtan Rivet]

On Mon, Aug 28, 2017 at 11:30:47AM +0100, Ferruh Yigit wrote:
> On 8/28/2017 10:55 AM, Gaëtan Rivet wrote:
> > Hi Raslan,
> > 
> > Redoing the review with the remarks from Thomas in mind.
> > 
> > On Wed, Aug 23, 2017 at 05:37:04PM +0300, Raslan Darawsheh wrote:
> >> Added hotplug in testpmd, to be able to test hotplug function
> >> in the PMD's.
> >>
> >> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> >> ---
> >>  app/test-pmd/cmdline.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >>  app/test-pmd/testpmd.c | 18 ++++++++++++++++++
> >>  app/test-pmd/testpmd.h |  1 +
> >>  3 files changed, 63 insertions(+)
> >>
> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> >> index cd8c358..b32a368 100644
> >> --- a/app/test-pmd/cmdline.c
> >> +++ b/app/test-pmd/cmdline.c
> >> @@ -716,6 +716,9 @@ static void cmd_help_long_parsed(void *parsed_result,
> >>  			"port config (port_id|all) l2-tunnel E-tag"
> >>  			" (enable|disable)\n"
> >>  			"    Enable/disable the E-tag support.\n\n"
> >> +
> >> +			" device remove (device)\n"
> > 
> > I think (device) might be hard to understand for a user.
> > Maybe (device name)?
> 
> I am suspicious on adding new root level command to testpmd, it is
> getting bigger, each command looks OK on its own context, but as a whole
> app getting more confusing [1].
> 
> Since dealing with device is kind of new, it can be OK to create new
> command tree, but there are already hotplug commands per port:
> "port attach #PCI|#VDEV_NAME"
> "port detach #P"
> 

Those two commands deal with the etherdev hotplug API.
The new command should test the rte_dev one.

As Thomas pointed out, the etherdev one deals only with eth ports, while
hotplug could be generalized to other devices, such as cryptodev.

> perhaps it can be good to keep "attach", "detach" keywords for device to
> be consistent?
> 
> "device attach #name"
> "device detach #name"
> 

I made a point of naming the hotplug operations in rte_bus plug/unplug
to avoid the confusion with the etherdev API. hotplug_add /
hotplug_remove also marks the distinction.

I don't know if it would be helpful for a developer writing a PMD,
searching for a way to test a functionality to have an API name
mismatch.

> Also a show equivalent can be added to work in device level:
> "show device info"
> 

I think it would be useful.

> 
> [1]
> http://dpdk.org/ml/archives/dev/2017-August/072764.html
> 
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Wu, Jingjing]

> >
> > Since dealing with device is kind of new, it can be OK to create new
> > command tree, but there are already hotplug commands per port:
> > "port attach #PCI|#VDEV_NAME"
> > "port detach #P"
> >
> 
> Those two commands deal with the etherdev hotplug API.
> The new command should test the rte_dev one.
> 
I was confused. The forwarding in testpmd setup is based on port id, right?
And all the devices listed in testpmd is etherdev, and all functional testings are all based on port id, right?
Then what is the difference to use port id or device name in testpmd?
And if no difference, what is the difference between detach and remove?

The only difference here I think is
Remove command is using rte_bus hotplug framework.
Attach/detach is using rte_eth_dev_detach API.

I think remove command should be an important command, and should not have ambiguity.
At least we need to doc it clearly.


> As Thomas pointed out, the etherdev one deals only with eth ports, while
> hotplug could be generalized to other devices, such as cryptodev.
> 
> > perhaps it can be good to keep "attach", "detach" keywords for device to
> > be consistent?
> >
> > "device attach #name"
> > "device detach #name"
> >
> 
> I made a point of naming the hotplug operations in rte_bus plug/unplug
> to avoid the confusion with the etherdev API. hotplug_add /
> hotplug_remove also marks the distinction.
> 
> I don't know if it would be helpful for a developer writing a PMD,
> searching for a way to test a functionality to have an API name
> mismatch.
> 
> > Also a show equivalent can be added to work in device level:
> > "show device info"
> >
> 
> I think it would be useful.
> 
> >
> > [1]
> > http://dpdk.org/ml/archives/dev/2017-August/072764.html
> >
> >
> 
> --
> Gaëtan Rivet
> 6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Ferruh Yigit]

On 9/7/2017 1:17 AM, Wu, Jingjing wrote:
>>>
>>> Since dealing with device is kind of new, it can be OK to create new
>>> command tree, but there are already hotplug commands per port:
>>> "port attach #PCI|#VDEV_NAME"
>>> "port detach #P"
>>>
>>
>> Those two commands deal with the etherdev hotplug API.
>> The new command should test the rte_dev one.
>>
> I was confused. The forwarding in testpmd setup is based on port id, right?
> And all the devices listed in testpmd is etherdev, and all functional testings are all based on port id, right?
> Then what is the difference to use port id or device name in testpmd?
> And if no difference, what is the difference between detach and remove?
> 
> The only difference here I think is
> Remove command is using rte_bus hotplug framework.
> Attach/detach is using rte_eth_dev_detach API.

Hi Raslan,

With latest code "detach" does rte_bus hotplug remove. So I believe this patch
is no more required. Can you please confirm?

Thanks,
ferruh

> 
> I think remove command should be an important command, and should not have ambiguity.
> At least we need to doc it clearly.
> 
> 
>> As Thomas pointed out, the etherdev one deals only with eth ports, while
>> hotplug could be generalized to other devices, such as cryptodev.
>>
>>> perhaps it can be good to keep "attach", "detach" keywords for device to
>>> be consistent?
>>>
>>> "device attach #name"
>>> "device detach #name"
>>>
>>
>> I made a point of naming the hotplug operations in rte_bus plug/unplug
>> to avoid the confusion with the etherdev API. hotplug_add /
>> hotplug_remove also marks the distinction.
>>
>> I don't know if it would be helpful for a developer writing a PMD,
>> searching for a way to test a functionality to have an API name
>> mismatch.
>>
>>> Also a show equivalent can be added to work in device level:
>>> "show device info"
>>>
>>
>> I think it would be useful.
>>
>>>
>>> [1]
>>> http://dpdk.org/ml/archives/dev/2017-August/072764.html
>>>
>>>
>>
>> --
>> Gaëtan Rivet
>> 6WIND
> 

Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

[Raslan Darawsheh]

Hi Ferruh,

Yes I believe this patch is no more required.

Kindest regards,
Raslan Darawsheh

-----Original Message-----
From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] 
Sent: Wednesday, November 29, 2017 12:02 AM
To: Wu, Jingjing <jingjing.wu@intel.com>; Gaëtan Rivet <gaetan.rivet@6wind.com>
Cc: Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Saleh Alsouqi <salehals@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: app/testpmd: add device removal command

On 9/7/2017 1:17 AM, Wu, Jingjing wrote:
>>>
>>> Since dealing with device is kind of new, it can be OK to create new 
>>> command tree, but there are already hotplug commands per port:
>>> "port attach #PCI|#VDEV_NAME"
>>> "port detach #P"
>>>
>>
>> Those two commands deal with the etherdev hotplug API.
>> The new command should test the rte_dev one.
>>
> I was confused. The forwarding in testpmd setup is based on port id, right?
> And all the devices listed in testpmd is etherdev, and all functional testings are all based on port id, right?
> Then what is the difference to use port id or device name in testpmd?
> And if no difference, what is the difference between detach and remove?
> 
> The only difference here I think is
> Remove command is using rte_bus hotplug framework.
> Attach/detach is using rte_eth_dev_detach API.

Hi Raslan,

With latest code "detach" does rte_bus hotplug remove. So I believe this patch is no more required. Can you please confirm?

Thanks,
ferruh

> 
> I think remove command should be an important command, and should not have ambiguity.
> At least we need to doc it clearly.
> 
> 
>> As Thomas pointed out, the etherdev one deals only with eth ports, 
>> while hotplug could be generalized to other devices, such as cryptodev.
>>
>>> perhaps it can be good to keep "attach", "detach" keywords for 
>>> device to be consistent?
>>>
>>> "device attach #name"
>>> "device detach #name"
>>>
>>
>> I made a point of naming the hotplug operations in rte_bus 
>> plug/unplug to avoid the confusion with the etherdev API. hotplug_add 
>> / hotplug_remove also marks the distinction.
>>
>> I don't know if it would be helpful for a developer writing a PMD, 
>> searching for a way to test a functionality to have an API name 
>> mismatch.
>>
>>> Also a show equivalent can be added to work in device level:
>>> "show device info"
>>>
>>
>> I think it would be useful.
>>
>>>
>>> [1]
>>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdp
>>> dk.org%2Fml%2Farchives%2Fdev%2F2017-August%2F072764.html&data=02%7C0
>>> 1%7Crasland%40mellanox.com%7Cdde3d3196af240dc46e508d536abb3fd%7Ca652
>>> 971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636475033483509610&sdata=tddN
>>> jwaa3H%2BNh2n7%2F%2BraWc82h9jNuFp9JXZx%2F%2BASH8M%3D&reserved=0
>>>
>>>
>>
>> --
>> Gaëtan Rivet
>> 6WIND
>