[dpdk-dev,v3,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 | 134 ++++++++++++++++++++++++++++++++++++++++++----
2 files changed, 125 insertions(+), 10 deletions(-)
Comments
On 19-Apr-18 5:50 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");
> + 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);
> + strncpy(ou->name, devname, RTE_DEV_NAME_MAX_LEN);
Probably better use strlcpy as it always null-terminates.
> + if (rte_mp_sendmsg(&mp_resp) < 0)
> + VDEV_LOG(ERR, "send vdev, %s, failed, %s",
> + devname, strerror(rte_errno));
> + num++;
Some comments on what is going on here (why are we sending messages in
response? why multiple? who will receive these messages?) would be nice.
I have a sneaking suspicion that you could've packed the response into
one single message, but i'm not completely sure what is going on here,
so maybe what you have here makes sense...
> + }
> + 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;
<...>
On 4/20/2018 4:41 PM, Burakov, Anatoly wrote:
> On 19-Apr-18 5:50 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");
>> + 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);
>> + strncpy(ou->name, devname, RTE_DEV_NAME_MAX_LEN);
>
> Probably better use strlcpy as it always null-terminates.
Yep.
>
>> + if (rte_mp_sendmsg(&mp_resp) < 0)
>> + VDEV_LOG(ERR, "send vdev, %s, failed, %s",
>> + devname, strerror(rte_errno));
>> + num++;
>
> Some comments on what is going on here (why are we sending messages in
> response? why multiple? who will receive these messages?) would be nice.
Yep, will explain that below.
> I have a sneaking suspicion that you could've packed the response into
> one single message, but i'm not completely sure what is going on here,
> so maybe what you have here makes sense...
What's happening here is that:
a. Secondary process sends a sync request to ask for vdev in primary.
b. Primary process receives the request, and send vdevs one by one.
c. Primary process sends back reply, which indicates how many vdevs are
sent.
The reason we don't pack all vdevs in the reply message is that, the
message length is RTE_MP_MAX_PARAM_LEN (256) in length. It's possible
that we cannot pack all vdevs in the single reply message.
Thanks,
Jianfeng
>> + }
>> + 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;
>
> <...>
>
On 20-Apr-18 3:28 PM, Tan, Jianfeng wrote:
>
>
> On 4/20/2018 4:41 PM, Burakov, Anatoly wrote:
>> On 19-Apr-18 5:50 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");
>>> + 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);
>>> + strncpy(ou->name, devname, RTE_DEV_NAME_MAX_LEN);
>>
>> Probably better use strlcpy as it always null-terminates.
>
> Yep.
>
>>
>>> + if (rte_mp_sendmsg(&mp_resp) < 0)
>>> + VDEV_LOG(ERR, "send vdev, %s, failed, %s",
>>> + devname, strerror(rte_errno));
>>> + num++;
>>
>> Some comments on what is going on here (why are we sending messages in
>> response? why multiple? who will receive these messages?) would be nice.
>
> Yep, will explain that below.
>
>> I have a sneaking suspicion that you could've packed the response into
>> one single message, but i'm not completely sure what is going on here,
>> so maybe what you have here makes sense...
>
> What's happening here is that:
>
> a. Secondary process sends a sync request to ask for vdev in primary.
> b. Primary process receives the request, and send vdevs one by one.
> c. Primary process sends back reply, which indicates how many vdevs are
> sent.
>
> The reason we don't pack all vdevs in the reply message is that, the
> message length is RTE_MP_MAX_PARAM_LEN (256) in length. It's possible
> that we cannot pack all vdevs in the single reply message.
>
OK. How does secondary know which vdevs are new and which aren't? Does
it even matter how many vdevs primary has sent? Correct me if i'm wrong,
but it seems that you're only using sync request as kind of
synchronization mechanism, and are not actually expecting any useful
data in the reply. Which is OK, but in that case just don't bother
sending any data in the reply in the first place :)
On 4/20/2018 11:19 PM, Burakov, Anatoly wrote:
> On 20-Apr-18 3:28 PM, Tan, Jianfeng wrote:
>>
>>
>> On 4/20/2018 4:41 PM, Burakov, Anatoly wrote:
>>> On 19-Apr-18 5:50 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");
>>>> + 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);
>>>> + strncpy(ou->name, devname, RTE_DEV_NAME_MAX_LEN);
>>>
>>> Probably better use strlcpy as it always null-terminates.
>>
>> Yep.
>>
>>>
>>>> + if (rte_mp_sendmsg(&mp_resp) < 0)
>>>> + VDEV_LOG(ERR, "send vdev, %s, failed, %s",
>>>> + devname, strerror(rte_errno));
>>>> + num++;
>>>
>>> Some comments on what is going on here (why are we sending messages
>>> in response? why multiple? who will receive these messages?) would
>>> be nice.
>>
>> Yep, will explain that below.
>>
>>> I have a sneaking suspicion that you could've packed the response
>>> into one single message, but i'm not completely sure what is going
>>> on here, so maybe what you have here makes sense...
>>
>> What's happening here is that:
>>
>> a. Secondary process sends a sync request to ask for vdev in primary.
>> b. Primary process receives the request, and send vdevs one by one.
>> c. Primary process sends back reply, which indicates how many vdevs
>> are sent.
>>
>> The reason we don't pack all vdevs in the reply message is that, the
>> message length is RTE_MP_MAX_PARAM_LEN (256) in length. It's possible
>> that we cannot pack all vdevs in the single reply message.
>>
>
> OK. How does secondary know which vdevs are new and which aren't?
This auto discovery is designed for secondary boot to know which vdevs
are used in primary. So they are all new to the secondary process. For
runtime vdev add in primary, we are going to rely on hotplug framework
to tell the news to secondary processes.
> Does it even matter how many vdevs primary has sent? Correct me if i'm
> wrong, but it seems that you're only using sync request as kind of
> synchronization mechanism, and are not actually expecting any useful
> data in the reply. Which is OK, but in that case just don't bother
> sending any data in the reply in the first place :)
I would like to keep this information, so that secondary process can
tell how many vdevs come from primary process (secondary process can
definitely iterate the vdev list to know, but it's that straightforward).
Thanks,
Jianfeng
On 20-Apr-18 4:32 PM, Tan, Jianfeng wrote:
>
>
> On 4/20/2018 11:19 PM, Burakov, Anatoly wrote:
>> On 20-Apr-18 3:28 PM, Tan, Jianfeng wrote:
>>>
>>>
>>> On 4/20/2018 4:41 PM, Burakov, Anatoly wrote:
>>>> On 19-Apr-18 5:50 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");
>>>>> + 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);
>>>>> + strncpy(ou->name, devname, RTE_DEV_NAME_MAX_LEN);
>>>>
>>>> Probably better use strlcpy as it always null-terminates.
>>>
>>> Yep.
>>>
>>>>
>>>>> + if (rte_mp_sendmsg(&mp_resp) < 0)
>>>>> + VDEV_LOG(ERR, "send vdev, %s, failed, %s",
>>>>> + devname, strerror(rte_errno));
>>>>> + num++;
>>>>
>>>> Some comments on what is going on here (why are we sending messages
>>>> in response? why multiple? who will receive these messages?) would
>>>> be nice.
>>>
>>> Yep, will explain that below.
>>>
>>>> I have a sneaking suspicion that you could've packed the response
>>>> into one single message, but i'm not completely sure what is going
>>>> on here, so maybe what you have here makes sense...
>>>
>>> What's happening here is that:
>>>
>>> a. Secondary process sends a sync request to ask for vdev in primary.
>>> b. Primary process receives the request, and send vdevs one by one.
>>> c. Primary process sends back reply, which indicates how many vdevs
>>> are sent.
>>>
>>> The reason we don't pack all vdevs in the reply message is that, the
>>> message length is RTE_MP_MAX_PARAM_LEN (256) in length. It's possible
>>> that we cannot pack all vdevs in the single reply message.
>>>
>>
>> OK. How does secondary know which vdevs are new and which aren't?
>
> This auto discovery is designed for secondary boot to know which vdevs
> are used in primary. So they are all new to the secondary process. For
> runtime vdev add in primary, we are going to rely on hotplug framework
> to tell the news to secondary processes.
>
>> Does it even matter how many vdevs primary has sent? Correct me if i'm
>> wrong, but it seems that you're only using sync request as kind of
>> synchronization mechanism, and are not actually expecting any useful
>> data in the reply. Which is OK, but in that case just don't bother
>> sending any data in the reply in the first place :)
>
> I would like to keep this information, so that secondary process can
> tell how many vdevs come from primary process (secondary process can
> definitely iterate the vdev list to know, but it's that straightforward).
>
OK, no strong objections here :)
@@ -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
@@ -196,8 +196,8 @@ alloc_devargs(const char *name, const char *args)
return devargs;
}
-int
-rte_vdev_init(const char *name, const char *args)
+static int
+insert_vdev(const char *name, const char *args, struct rte_vdev_device **p_dev)
{
struct rte_vdev_device *dev;
struct rte_devargs *devargs;
@@ -229,6 +229,33 @@ rte_vdev_init(const char *name, const char *args)
TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
rte_spinlock_unlock(&vdev_device_list_lock);
+ TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
+
+ if (p_dev)
+ *p_dev = dev;
+
+ return 0;
+
+fail:
+ free(devargs->args);
+ free(devargs);
+ free(dev);
+ return ret;
+}
+
+int
+rte_vdev_init(const char *name, const char *args)
+{
+ struct rte_vdev_device *dev;
+ struct rte_devargs *devargs;
+ int ret;
+
+ ret = insert_vdev(name, args, &dev);
+ if (ret < 0)
+ return ret;
+
+ devargs = dev->device.devargs;
+
ret = vdev_probe_all_drivers(dev);
if (ret) {
if (ret > 0)
@@ -237,17 +264,14 @@ rte_vdev_init(const char *name, const char *args)
rte_spinlock_lock(&vdev_device_list_lock);
TAILQ_REMOVE(&vdev_device_list, dev, next);
rte_spinlock_unlock(&vdev_device_list_lock);
- goto fail;
- }
- TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
+ TAILQ_REMOVE(&devargs_list, devargs, next);
- return 0;
+ free(devargs->args);
+ free(devargs);
+ free(dev);
+ }
-fail:
- free(devargs->args);
- free(devargs);
- free(dev);
return ret;
}
@@ -305,6 +329,68 @@ rte_vdev_uninit(const char *name)
return 0;
}
+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);
+
+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);
+ strncpy(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)
{
@@ -312,6 +398,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) {