Hi Anatoly:
> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Monday, June 18, 2018 6:36 PM
> To: Zhang, Qi Z <qi.z.zh...@intel.com>; tho...@monjalon.net
> Cc: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richard...@intel.com>; Yigit, Ferruh
> <ferruh.yi...@intel.com>; Shelton, Benjamin H
> <benjamin.h.shel...@intel.com>; Vangati, Narender
> <narender.vang...@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.zh...@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
