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

[Zhang, Qi Z]

Hi Anatoly:

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Monday, June 18, 2018 6:36 PM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>; thomas at monjalon.net
> Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org;
> Richardson, Bruce <bruce.richardson at intel.com>; Yigit, Ferruh
> <ferruh.yigit at intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton at intel.com>; Vangati, Narender
> <narender.vangati at intel.com>
> Subject: Re: [PATCH 22/22] examples/devmgm_mp: add simple device
> management sample
> 
> 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 at 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)


Ok, I saw all the multi-process samples are in examples/multi_process, so I think this the right place to add
it could be "hotplug_mp" to follow other samples naming rule.
> 
> >   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?

OK, this is copy from exist sample code :), I will clean up the header file in v3.

> 
> 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"

Got it, thanks

> 
> > +
> > +/**********************************************************/
> > +
> > +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.

All this comment should be removed since TODO already done :)
Actually, we guarantee device be detached from secondary before primary.

> 
> > +		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.

OK, will add it.

Thanks
Qi

> 
> --
> Thanks,
> Anatoly