From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id D4C3C3572 for ; Tue, 10 Jul 2018 16:01:04 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Jul 2018 07:01:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,334,1526367600"; d="scan'208";a="73644422" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.102]) ([10.237.220.102]) by orsmga002.jf.intel.com with ESMTP; 10 Jul 2018 07:01:00 -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> <20180709033706.27858-1-qi.z.zhang@intel.com> <20180709033706.27858-6-qi.z.zhang@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Tue, 10 Jul 2018 15:00:59 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.0 MIME-Version: 1.0 In-Reply-To: <20180709033706.27858-6-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 v10 05/19] eal: enable hotplug on multi-process 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: Tue, 10 Jul 2018 14:01:06 -0000 On 09-Jul-18 4:36 AM, Qi Zhang wrote: > We are going to introduce the solution to handle hotplug in > multi-process, it includes the below scenario: > > 1. Attach a device from the primary > 2. Detach a device from the primary > 3. Attach a device from a secondary > 4. Detach a device from a secondary > > In the primary-secondary process model, we assume devices are shared > by default. that means attaches or detaches a device on any process > will broadcast to all other processes through mp channel then device > information will be synchronized on all processes. > > Any failure during attaching/detaching process will cause inconsistent > status between processes, so proper rollback action should be considered. > > This patch covers the implementation of case 1,2. > Case 3,4 will be implemented on a separate patch. > > IPC scenario for Case 1, 2: > > attach a device > a) primary attach the new device if failed goto h). > b) primary send attach sync request to all secondary. > c) secondary receive request and attach the device and send a reply. > d) primary check the reply if all success goes to i). > e) primary send attach rollback sync request to all secondary. > f) secondary receive the request and detach the device and send a reply. > g) primary receive the reply and detach device as rollback action. > h) attach fail > i) attach success > > detach a device > a) primary send detach sync request to all secondary > b) secondary detach the device and send reply > c) primary check the reply if all success goes to f). > d) primary send detach rollback sync request to all secondary. > e) secondary receive the request and attach back device. goto g) > f) primary detach the device if success goto g), else goto d) > g) detach fail. > h) detach success. > > Signed-off-by: Qi Zhang > --- > + req.t = EAL_DEV_REQ_TYPE_ATTACH; > + strlcpy(req.busname, busname, RTE_BUS_NAME_MAX_LEN); > + strlcpy(req.devname, devname, RTE_DEV_NAME_MAX_LEN); > + strlcpy(req.devargs, devargs, RTE_DEV_ARGS_MAX_LEN); > + > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) > + return -ENOTSUP; Nitpick, but maybe do this before strlcpy? > + > + /** > + * attach a device from primary start from here: > + * > + * a) primary attach the new device if failed goto h). > + * b) primary send attach sync request to all secondary. > + * c) secondary receive request and attach the device and send a reply. > + * d) primary check the reply if all success goes to i). > + > + memset(&req, 0, sizeof(req)); > + req.t = EAL_DEV_REQ_TYPE_DETACH; > + strlcpy(req.busname, busname, RTE_BUS_NAME_MAX_LEN); > + strlcpy(req.devname, devname, RTE_DEV_NAME_MAX_LEN); > + > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) > + return -ENOTSUP; Same nitpick, probably move this above. > + > + /** > + * detach a device from primary start from here: > + * > + * a) primary send detach sync request to all secondary > + * b) secondary detach the device and send reply > + struct mp_reply_bundle *bundle = param; > + struct rte_mp_msg *msg = &bundle->msg; > + const struct eal_dev_mp_req *req = > + (const struct eal_dev_mp_req *)msg->param; > + struct rte_mp_msg mp_resp; > + struct eal_dev_mp_req *resp = > + (struct eal_dev_mp_req *)mp_resp.param; > + int ret = 0; > + > + memset(&mp_resp, 0, sizeof(mp_resp)); > + > + switch (req->t) { > + case EAL_DEV_REQ_TYPE_ATTACH: > + case EAL_DEV_REQ_TYPE_DETACH_ROLLBACK: > + ret = do_dev_hotplug_add(req->busname, req->devname, ""); I'm not too familiar with devargs and hotplug, but why are we passing empty devargs string here? Is it possible for it to be not empty? > + break; > + case EAL_DEV_REQ_TYPE_DETACH: > + case EAL_DEV_REQ_TYPE_ATTACH_ROLLBACK: > + ret = do_dev_hotplug_remove(req->busname, req->devname); > + break; > + default: > + ret = -EINVAL; > + } > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) { > + ret = rte_mp_action_register(EAL_DEV_MP_ACTION_REQUEST, > + handle_secondary_request); > + if (ret) { > + RTE_LOG(ERR, EAL, "Couldn't register '%s' action\n", > + EAL_DEV_MP_ACTION_REQUEST); > + return ret; > + } > + } else { > + ret = rte_mp_action_register(EAL_DEV_MP_ACTION_REQUEST, > + handle_primary_request); ^^ wrong indentation. > + if (ret) { > + RTE_LOG(ERR, EAL, "Couldn't register '%s' action\n", > + EAL_DEV_MP_ACTION_REQUEST); > + return ret; > + } > + } > + > +#endif /* _HOTPLUG_MP_H_ */ > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h > index eb9eded4e..720f7c3c8 100644 > --- a/lib/librte_eal/common/include/rte_bus.h > +++ b/lib/librte_eal/common/include/rte_bus.h > @@ -197,6 +197,9 @@ struct rte_bus_conf { > typedef enum rte_iova_mode (*rte_bus_get_iommu_class_t)(void); > > > +/* Max length for a bus name */ > +#define RTE_BUS_NAME_MAX_LEN 32 Is this enforced anywhere in the bus codebase? Can we guarantee that bus name will never be bigger than this? > + > /** > * A structure describing a generic bus. > */ > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h > index 3879ff3ca..667df20f0 100644 > --- a/lib/librte_eal/common/include/rte_dev.h > +++ b/lib/librte_eal/common/include/rte_dev.h > @@ -152,6 +152,9 @@ struct rte_driver { > */ > #define RTE_DEV_NAME_MAX_LEN 64 > > +/* Max devargs length be allowed */ > +#define RTE_DEV_ARGS_MAX_LEN 128 Same - is this enforced anywhere in the codebase related to devargs? -- Thanks, Anatoly