From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 43A3A1559 for ; Mon, 18 Jun 2018 12:36:19 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jun 2018 03:36:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,238,1526367600"; d="scan'208";a="65032603" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.252.30.40]) ([10.252.30.40]) by fmsmga001.fm.intel.com with ESMTP; 18 Jun 2018 03:36:15 -0700 To: Qi Zhang , thomas@monjalon.net Cc: konstantin.ananyev@intel.com, dev@dpdk.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, benjamin.h.shelton@intel.com, narender.vangati@intel.com References: <20180607123849.14439-1-qi.z.zhang@intel.com> <20180607123849.14439-23-qi.z.zhang@intel.com> From: "Burakov, Anatoly" Message-ID: <181b45a2-93cc-5700-5f9e-2dfdf474e38c@intel.com> Date: Mon, 18 Jun 2018 11:36:15 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180607123849.14439-23-qi.z.zhang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 22/22] examples/devmgm_mp: add simple device management sample X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jun 2018 10:36:20 -0000 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 > --- 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 > > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#ifndef __linux__ > + #ifdef __FreeBSD__ > + #include > + #else > + #include > + #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 > +#include > +#include > +#include > +#include > +#include > +#include 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 #include #include #include #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, > +{ > + 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"); > + } > +#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