[dpdk-dev,v4,3/5] bus/vdev: bus scan by multi-process channel
Checks
Commit Message
To scan the vdevs in primary, we send request to primary process
to obtain the names for vdevs.
Only the name is shared from the primary. In probe(), the device
driver is supposed to locate (or request more) the detail
information from the primary.
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
---
drivers/bus/vdev/Makefile | 1 +
drivers/bus/vdev/vdev.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
Comments
On 20-Apr-18 5:57 PM, Jianfeng Tan wrote:
> To scan the vdevs in primary, we send request to primary process
> to obtain the names for vdevs.
>
> Only the name is shared from the primary. In probe(), the device
> driver is supposed to locate (or request more) the detail
> information from the primary.
>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
<...>
> +static int
> +vdev_action(const struct rte_mp_msg *mp_msg, const void *peer)
> +{
> + struct rte_vdev_device *dev;
> + struct rte_mp_msg mp_resp;
> + struct vdev_param *ou = (struct vdev_param *)&mp_resp.param;
> + const struct vdev_param *in = (const struct vdev_param *)mp_msg->param;
> + const char *devname;
> + int num;
> +
> + strcpy(mp_resp.name, "vdev");
This string is used in a lot of places, so... #define? also, i think
action name is a bit too short. maybe make it more descriptive, like
"bus_vdev" or something to that effect?
Also, i think Coverity will complain about not checking string length,
so... strlcpy()?
> + mp_resp.len_param = sizeof(*ou);
> + mp_resp.num_fds = 0;
> +
> + switch (in->type) {
> + case VDEV_SCAN_REQ:
> + ou->type = VDEV_SCAN_ONE;
> + ou->num = 1;
> + num = 0;
> +
> + rte_spinlock_lock(&vdev_device_list_lock);
> + TAILQ_FOREACH(dev, &vdev_device_list, next) {
> + devname = rte_vdev_device_name(dev);
> + if (strlen(devname) == 0)
> + VDEV_LOG(INFO, "vdev with no name is not sent");
The comment says it's "not sent" but code doesn't seem to indicate that
this will happen. Forgot "continue"?
> + VDEV_LOG(INFO, "send vdev, %s", devname);
> + strlcpy(ou->name, devname, RTE_DEV_NAME_MAX_LEN);
> + if (rte_mp_sendmsg(&mp_resp) < 0)
> + VDEV_LOG(ERR, "send vdev, %s, failed, %s",
> + devname, strerror(rte_errno));
> + num++;
> + }
Once all of that is addressed,
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Monday, April 23, 2018 5:55 PM
> To: Tan, Jianfeng; dev@dpdk.org
> Cc: thomas@monjalon.net
> Subject: Re: [dpdk-dev] [PATCH v4 3/5] bus/vdev: bus scan by multi-process
> channel
>
> On 20-Apr-18 5:57 PM, Jianfeng Tan wrote:
> > To scan the vdevs in primary, we send request to primary process
> > to obtain the names for vdevs.
> >
> > Only the name is shared from the primary. In probe(), the device
> > driver is supposed to locate (or request more) the detail
> > information from the primary.
> >
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
>
> <...>
>
> > +static int
> > +vdev_action(const struct rte_mp_msg *mp_msg, const void *peer)
> > +{
> > + struct rte_vdev_device *dev;
> > + struct rte_mp_msg mp_resp;
> > + struct vdev_param *ou = (struct vdev_param *)&mp_resp.param;
> > + const struct vdev_param *in = (const struct vdev_param *)mp_msg-
> >param;
> > + const char *devname;
> > + int num;
> > +
> > + strcpy(mp_resp.name, "vdev");
>
> This string is used in a lot of places, so... #define? also, i think
> action name is a bit too short. maybe make it more descriptive, like
> "bus_vdev" or something to that effect?
>
> Also, i think Coverity will complain about not checking string length,
> so... strlcpy()?
Will do in next version.
>
> > + mp_resp.len_param = sizeof(*ou);
> > + mp_resp.num_fds = 0;
> > +
> > + switch (in->type) {
> > + case VDEV_SCAN_REQ:
> > + ou->type = VDEV_SCAN_ONE;
> > + ou->num = 1;
> > + num = 0;
> > +
> > + rte_spinlock_lock(&vdev_device_list_lock);
> > + TAILQ_FOREACH(dev, &vdev_device_list, next) {
> > + devname = rte_vdev_device_name(dev);
> > + if (strlen(devname) == 0)
> > + VDEV_LOG(INFO, "vdev with no name is not
> sent");
>
> The comment says it's "not sent" but code doesn't seem to indicate that
> this will happen. Forgot "continue"?
Nice catch, will fix.
Thanks,
Jianfeng
>
> > + VDEV_LOG(INFO, "send vdev, %s", devname);
> > + strlcpy(ou->name, devname,
> RTE_DEV_NAME_MAX_LEN);
> > + if (rte_mp_sendmsg(&mp_resp) < 0)
> > + VDEV_LOG(ERR, "send vdev, %s, failed, %s",
> > + devname, strerror(rte_errno));
> > + num++;
> > + }
>
> Once all of that is addressed,
>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
> --
> Thanks,
> Anatoly
@@ -10,6 +10,7 @@ LIB = librte_bus_vdev.a
CFLAGS += -O3
CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -DALLOW_EXPERIMENTAL_API
# versioning export map
EXPORT_MAP := rte_bus_vdev_version.map
@@ -18,6 +18,7 @@
#include <rte_memory.h>
#include <rte_tailq.h>
#include <rte_spinlock.h>
+#include <rte_string_fns.h>
#include <rte_errno.h>
#include "rte_bus_vdev.h"
@@ -316,6 +317,77 @@ rte_vdev_uninit(const char *name)
return ret;
}
+struct vdev_param {
+#define VDEV_SCAN_REQ 1
+#define VDEV_SCAN_ONE 2
+#define VDEV_SCAN_REP 3
+ int type;
+ int num;
+ char name[RTE_DEV_NAME_MAX_LEN];
+};
+
+static int vdev_plug(struct rte_device *dev);
+
+/**
+ * This function works as the action for both primary and secondary process
+ * for static vdev discovery when a secondary process is booting.
+ *
+ * step 1, secondary process sends a sync request to ask for vdev in primary;
+ * step 2, primary process receives the request, and send vdevs one by one;
+ * step 3, primary process sends back reply, which indicates how many vdevs
+ * are sent.
+ */
+static int
+vdev_action(const struct rte_mp_msg *mp_msg, const void *peer)
+{
+ struct rte_vdev_device *dev;
+ struct rte_mp_msg mp_resp;
+ struct vdev_param *ou = (struct vdev_param *)&mp_resp.param;
+ const struct vdev_param *in = (const struct vdev_param *)mp_msg->param;
+ const char *devname;
+ int num;
+
+ strcpy(mp_resp.name, "vdev");
+ mp_resp.len_param = sizeof(*ou);
+ mp_resp.num_fds = 0;
+
+ switch (in->type) {
+ case VDEV_SCAN_REQ:
+ ou->type = VDEV_SCAN_ONE;
+ ou->num = 1;
+ num = 0;
+
+ rte_spinlock_lock(&vdev_device_list_lock);
+ TAILQ_FOREACH(dev, &vdev_device_list, next) {
+ devname = rte_vdev_device_name(dev);
+ if (strlen(devname) == 0)
+ VDEV_LOG(INFO, "vdev with no name is not sent");
+ VDEV_LOG(INFO, "send vdev, %s", devname);
+ strlcpy(ou->name, devname, RTE_DEV_NAME_MAX_LEN);
+ if (rte_mp_sendmsg(&mp_resp) < 0)
+ VDEV_LOG(ERR, "send vdev, %s, failed, %s",
+ devname, strerror(rte_errno));
+ num++;
+ }
+ rte_spinlock_unlock(&vdev_device_list_lock);
+
+ ou->type = VDEV_SCAN_REP;
+ ou->num = num;
+ if (rte_mp_reply(&mp_resp, peer) < 0)
+ VDEV_LOG(ERR, "Failed to reply a scan request");
+ break;
+ case VDEV_SCAN_ONE:
+ VDEV_LOG(INFO, "receive vdev, %s", in->name);
+ if (insert_vdev(in->name, NULL, NULL) < 0)
+ VDEV_LOG(ERR, "failed to add vdev, %s", in->name);
+ break;
+ default:
+ VDEV_LOG(ERR, "vdev cannot recognize this message");
+ }
+
+ return 0;
+}
+
static int
vdev_scan(void)
{
@@ -323,6 +395,34 @@ vdev_scan(void)
struct rte_devargs *devargs;
struct vdev_custom_scan *custom_scan;
+ if (rte_mp_action_register("vdev", vdev_action) < 0 &&
+ rte_errno != EEXIST) {
+ VDEV_LOG(ERR, "vdev fails to add action");
+ return -1;
+ }
+
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+ struct rte_mp_msg mp_req, *mp_rep;
+ struct rte_mp_reply mp_reply;
+ struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
+ struct vdev_param *req = (struct vdev_param *)mp_req.param;
+ struct vdev_param *resp;
+
+ strcpy(mp_req.name, "vdev");
+ mp_req.len_param = sizeof(*req);
+ mp_req.num_fds = 0;
+ req->type = VDEV_SCAN_REQ;
+ if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) == 0 &&
+ mp_reply.nb_received == 1) {
+ mp_rep = &mp_reply.msgs[0];
+ resp = (struct vdev_param *)mp_rep->param;
+ VDEV_LOG(INFO, "Received %d vdevs", resp->num);
+ } else
+ VDEV_LOG(ERR, "Failed to request vdev from primary");
+
+ /* Fall through to allow private vdevs in secondary process */
+ }
+
/* call custom scan callbacks if any */
rte_spinlock_lock(&vdev_custom_scan_lock);
TAILQ_FOREACH(custom_scan, &vdev_custom_scans, next) {