[dpdk-dev] [PATCH V15 2/5] eal: add uevent pass and process function

[Guo, Jia]

jianfeng, thanks for your review. almost make sense and comment as bellow.


On 3/21/2018 10:20 PM, Tan, Jianfeng wrote:
>
>
> On 3/21/2018 1:27 PM, Jeff Guo wrote:
>> In order to handle the uevent which have been detected from the kernel
>> side, add uevent process function, let hot plug event to be example to
>> show uevent mechanism how to pass the uevent and process the uevent.
>
> In fact, how to pass the uevent to eal/linux for processing, is 
> already done by last patch, by registering a callback into interrupt 
> thread.
>
> In this patch, we are actually showing how to process the uevent, and 
> translate it into RTE_DEV_EVENT_ADD, RTE_DEV_EVENT_DEL, etc.
>
> So the title would be something like:
>
> eal/linux: translate uevent to dev event
>
>
sorry, that what i mean should be uevent message parse but not pass, and 
what you say make sense.
>>
>> About uevent passing and processing, add below functions in linux eal
>> dev layer. FreeBSD not support uevent ,so let it to be void and do not
>> implement in function.
>> a.dev_uev_parse
>> b.dev_uev_receive
>> c.dev_uev_process
>
> We already have dummy rte_dev_event_monitor_start and 
> rte_dev_event_monitor_stop, we don't need to have those dummy internal 
> functions any more. Actually, you did not implement those dummy 
> functions.
>
yes, not dummy just internal function.
>>
>> Signed-off-by: Jeff Guo <jia.guo at intel.com>
>> ---
>> v15->v14:
>> remove the uevent type check and any policy from eal,
>> let it check and management in user's callback.
>> ---
>>   lib/librte_eal/common/include/rte_dev.h | 17 ++++++
>
> And if you agree with me in the above, we shall not touch this file. 
> Move the definition into the previous patch.
>
will check and split the definition more explicit.
>>   lib/librte_eal/linuxapp/eal/eal_dev.c | 95 
>> ++++++++++++++++++++++++++++++++-
>>   2 files changed, 111 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/common/include/rte_dev.h 
>> b/lib/librte_eal/common/include/rte_dev.h
>> index d2fcbc9..98ea12b 100644
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -24,6 +24,23 @@ extern "C" {
>>   #include <rte_compat.h>
>>   #include <rte_log.h>
>>   +#define RTE_EAL_UEV_MSG_LEN 4096
>> +#define RTE_EAL_UEV_MSG_ELEM_LEN 128
>
> Such macro shall be linux uevent specific, so put them in linuxapp 
> folder.
>
agree.
>> +
>> +enum rte_dev_state {
>> +    RTE_DEV_UNDEFINED,    /**< unknown device state */
>> +    RTE_DEV_FAULT,    /**< device fault or error */
>> +    RTE_DEV_PARSED,    /**< device has been scanned on bus*/
>> +    RTE_DEV_PROBED,    /**< device has been probed driver */
>> +};
>
> This enum is not used in this patch series, I do see it's used in the 
> other series. So put the definition there.
>
yes.
>> +
>> +enum rte_dev_event_subsystem {
>> +    RTE_DEV_EVENT_SUBSYSTEM_UNKNOWN,
>
> I don't see where we use this macro. Seems that we now only implement 
> UIO, so I suppose, we shall set the other cases to this UNKNOWN.
>
ok.
>> +    RTE_DEV_EVENT_SUBSYSTEM_UIO,
>> +    RTE_DEV_EVENT_SUBSYSTEM_VFIO,
>
> If we don't support VFIO now, I prefer not defining it now.
>
will remove it at this stage and add later.
>> +    RTE_DEV_EVENT_SUBSYSTEM_MAX
>> +};
>
>> +
>>   /**
>>    * The device event type.
>>    */
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c 
>> b/lib/librte_eal/linuxapp/eal/eal_dev.c
>> index 9d9e088..2b34e2c 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
>> @@ -78,9 +78,102 @@ dev_uev_monitor_create(int netlink_fd)
>>   }
>>     static void
>> +dev_uev_parse(const char *buf, struct rte_dev_event *event)
>> +{
>> +    char action[RTE_EAL_UEV_MSG_ELEM_LEN];
>> +    char subsystem[RTE_EAL_UEV_MSG_ELEM_LEN];
>> +    char dev_path[RTE_EAL_UEV_MSG_ELEM_LEN];
>> +    char pci_slot_name[RTE_EAL_UEV_MSG_ELEM_LEN];
>> +    int i = 0;
>> +
>> +    memset(action, 0, RTE_EAL_UEV_MSG_ELEM_LEN);
>> +    memset(subsystem, 0, RTE_EAL_UEV_MSG_ELEM_LEN);
>> +    memset(dev_path, 0, RTE_EAL_UEV_MSG_ELEM_LEN);
>> +    memset(pci_slot_name, 0, RTE_EAL_UEV_MSG_ELEM_LEN);
>> +
>
> Maybe we can put an example here for better understanding.
>
> And if this buf can contain multiple events? If yes, the 
> implementation is not correct, we will only record one event; if no, 
> we can simplify it a little bit.
>
the buf do not contain multiple event but will involve more string split 
by several  "/0" , so need check that by bellow code.
>> +    while (i < RTE_EAL_UEV_MSG_LEN) {
>> +        for (; i < RTE_EAL_UEV_MSG_LEN; i++) {
>> +            if (*buf)
>> +                break;
>> +            buf++;
>> +        }
>
> If we pass in the length of the buf, we don't have to skip "\0"?
>
the reason is show as above.
>> +        /**
>> +         * check device uevent from kernel side, no need to check
>> +         * uevent from udev.
>> +         */
>> +        if (!strncmp(buf, "libudev", 7)) {
>
> Use strcmp is enough. And we actually need to check left length enough 
> for strlen("libudev").
>
>> +            buf += 7;
>> +            i += 7;
>> +            return;
>> +        }
>> +        if (!strncmp(buf, "ACTION=", 7)) {
>> +            buf += 7;
>> +            i += 7;
>> +            snprintf(action, sizeof(action), "%s", buf);
>> +        } else if (!strncmp(buf, "DEVPATH=", 8)) {
>> +            buf += 8;
>> +            i += 8;
>> +            snprintf(dev_path, sizeof(dev_path), "%s", buf);
>> +        } else if (!strncmp(buf, "SUBSYSTEM=", 10)) {
>> +            buf += 10;
>> +            i += 10;
>> +            snprintf(subsystem, sizeof(subsystem), "%s", buf);
>> +        } else if (!strncmp(buf, "PCI_SLOT_NAME=", 14)) {
>> +            buf += 14;
>> +            i += 14;
>> +            snprintf(pci_slot_name, sizeof(subsystem), "%s", buf);
>> +            event->devname = pci_slot_name;
>> +        }
>> +        for (; i < RTE_EAL_UEV_MSG_LEN; i++) {
>> +            if (*buf == '\0')
>> +                break;
>> +            buf++;
>> +        }
>
> As we already check '\0' in the begin of the loop, we don't need it at 
> the end any more.
>
the reason is show as above.
>> +    }
>> +
>> +    if ((!strncmp(subsystem, "uio", 3)) ||
>> +        (!strncmp(subsystem, "pci", 3)))
>> +        event->subsystem = RTE_DEV_EVENT_SUBSYSTEM_UIO;
>> +    if (!strncmp(action, "add", 3))
>> +        event->type = RTE_DEV_EVENT_ADD;
>> +    if (!strncmp(action, "remove", 6))
>> +        event->type = RTE_DEV_EVENT_REMOVE;
>> +}
>> +
>> +static int
>> +dev_uev_receive(int fd, struct rte_dev_event *uevent)
>> +{
>> +    int ret;
>> +    char buf[RTE_EAL_UEV_MSG_LEN];
>> +
>> +    memset(uevent, 0, sizeof(struct rte_dev_event));
>> +    memset(buf, 0, RTE_EAL_UEV_MSG_LEN);
>> +
>> +    ret = recv(fd, buf, RTE_EAL_UEV_MSG_LEN - 1, MSG_DONTWAIT);
>> +    if (ret < 0) {
>> +        RTE_LOG(ERR, EAL,
>> +        "Socket read error(%d): %s\n",
>> +        errno, strerror(errno));
>> +        return -1;
>> +    } else if (ret == 0)
>> +        /* connection closed */
>> +        return -1;
>
> So we are sure how many bytes shall be parsed, we can pass the length 
> into dev_uev_parse().
>
might be better from what you said.
>> +
>> +    dev_uev_parse(buf, uevent);
>> +
>> +    return 0;
>> +}
>> +
>> +static void
>>   dev_uev_process(__rte_unused void *param)
>>   {
>> -    /* TODO: device uevent processing */
>> +    struct rte_dev_event uevent;
>> +
>> +    if (dev_uev_receive(intr_handle.fd, &uevent))
>> +        return;
>
> We don't use uevent->subsystem below, why we have to define it in 
> first place?
>
could check here and i will add that check only for uio now.
>> +
>> +    if (uevent.devname)
>> +        _rte_dev_callback_process(uevent.devname, uevent.type, NULL);
>>   }
>>     int __rte_experimental
>