Re: [dpdk-dev] [PATCH 22/22] examples/devmgm_mp: add simple device management sample

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 07-Jun-18 1:38 PM, Qi Zhang wrote:
> The sample code demonstrate device (ethdev only) management
> at multi-process envrionment. User can attach/detach a device
> on primary process and see it is synced on secondary process
> automatically, also user can lock a device to prevent it be
> detached or unlock it to go back to default behaviour.
> 
> How to start?
> ./devmgm_mp --proc-type=auto
> 
> Command Line Example:
> 
>> help
>> list
> 
> /* attach a af_packet vdev */
>> attach net_af_packet,iface=eth0
> 
> /* detach port 0 */
>> detach 0
> 
> /* attach a private af_packet vdev (secondary process only)*/
>> attachp net_af_packet,iface=eth0
> 
> /* detach a private device (secondary process only) */
>> detachp 0
> 
> /* lock port 0 */
>> lock 0
> 
> /* unlock port 0 */
>> unlock 0
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---

I think the "devmgm_mp" is not a descriptive enough name. What this 
example demonstrates, is device hotplug. So how about naming the example 
app "hotplug"? (or "mp_hotplug" to indicate that it specifically sets 
out to demonstrate multiprocess hotplug)

>   examples/devmgm_mp/Makefile    |  64 +++++++
>   examples/devmgm_mp/commands.c  | 383 +++++++++++++++++++++++++++++++++++++++++
>   examples/devmgm_mp/commands.h  |  10 ++
>   examples/devmgm_mp/main.c      |  41 +++++
>   examples/devmgm_mp/meson.build |  11 ++
>   5 files changed, 509 insertions(+)
>   create mode 100644 examples/devmgm_mp/Makefile
>   create mode 100644 examples/devmgm_mp/commands.c
>   create mode 100644 examples/devmgm_mp/commands.h
>   create mode 100644 examples/devmgm_mp/main.c
>   create mode 100644 examples/devmgm_mp/meson.build
> 

<snip>

> +#include <stdio.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <errno.h>
> +#include <netinet/in.h>
> +#include <termios.h>
> +#ifndef __linux__
> +	#ifdef __FreeBSD__
> +		#include <sys/socket.h>
> +	#else
> +		#include <net/socket.h>
> +	#endif
> +#endif

This seems like a weird define. Care to elaborate why are we checking 
for __linux__ not being defined?

If you're trying to differentiate between Linux and FreeBSD, there's a 
readly RTE_EXEC_ENV_* config options, e.g.

#ifdef RTE_EXEC_ENV_LINUXAPP
// linux defines
#endif
#ifdef RTE_EXEC_ENV_BSDAPP
// bsd defines
#endif

or something to that effect.

> +
> +#include <cmdline_rdline.h>
> +#include <cmdline_parse.h>
> +#include <cmdline_parse_ipaddr.h>
> +#include <cmdline_parse_num.h>
> +#include <cmdline_parse_string.h>
> +#include <cmdline.h>
> +#include <rte_ethdev.h>

Generally (and as per DPDK coding guidelines), we prefer defines ordered 
as follows:

1) system defines enclosed in brackets
2) DPDK defines (rte_blah) enclosed in brackets
3) private/application-specific defines enclosed in quotes.

All three groups should be separated by newline.

So, these defines should've read as:

#include <stdblah.h>
#include <sys/blah.h>

#include <rte_blah.h>
#include <rte_foo.h>

#include "cmdline_blah.h"
#include "cmdline_foo.h"

> +
> +/**********************************************************/
> +
> +struct cmd_help_result {
> +	cmdline_fixed_string_t help;
> +};
> +
> +static void cmd_help_parsed(__attribute__((unused)) void *parsed_result,

<snip>

> +{
> +	uint16_t port_id;
> +	char dev_name[RTE_DEV_NAME_MAX_LEN];
> +
> +	cmdline_printf(cl, "list all etherdev\n");
> +
> +	RTE_ETH_FOREACH_DEV(port_id) {
> +		rte_eth_dev_get_name_by_port(port_id, dev_name);
> +		/* Secondary process's ethdev->state may not be
> +		 * updated after detach on primary process,  but
> +		 * ethdev->data should already be reset, so
> +		 * use strlen(dev_name) == 0 to know the port is
> +		 * not used.
> +		 *
> +		 * TODO: Secondary process should be informed when a
> +		 * port is released on primary through mp channel.
> +		 */

That seems like a weird thing to leave out for TODO - it looks like an 
API deficiency. Can this be automatically updated on multiprocess 
hotplug sync, or somehow managed inside RTE_ETH_FOREACH_DEV?

As i understand, per-process ethdev list is not protected by any locks, 
so doing this is racy. Since this is a multiprocess hotplug example app, 
it should demonstrate best practices. So, either RTE_ETH_FOREACH_DEV 
should be fixed to handle this case, or the application should 
demonstrate how to properly synchronize access to local device list. The 
latter is probably better as adding locking around ethdev device list is 
outside the scope of this patchset.

> +		if (strlen(dev_name) > 0)
> +			cmdline_printf(cl, "%d\t%s\n", port_id, dev_name);
> +		else
> +			printf("empty dev_name is not expected!\n");
> +	}

<snip>

> +#include "commands.h"
> +
> +int main(int argc, char **argv)
> +{
> +	int ret;
> +	struct cmdline *cl;
> +
> +	ret = rte_eal_init(argc, argv);
> +	if (ret < 0)
> +		rte_panic("Cannot init EAL\n");
> +
> +	cl = cmdline_stdin_new(main_ctx, "example> ");
> +	if (cl == NULL)
> +		rte_panic("Cannot create cmdline instance\n");
> +	cmdline_interact(cl);
> +	cmdline_stdin_exit(cl);
> +
> +	return 0;

Application should call rte_eal_cleanup() before exit. Otherwise, each 
secondary started and stopped will leak memory.

-- 
Thanks,
Anatoly