| Message ID | 20221104102327.770260-2-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | David Ahern |
| Headers | show |
| Series | devlink: make ifname-port relationship behaviour better | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 11/4/22 4:23 AM, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > ifname map is created once during init. However, ifnames can easily > change during the devlink process runtime (e. g. devlink mon). why not update the cache on name changes? Doing a query on print has extra overhead. And, if you insist a per-print query is needed, why leave ifname_map_list? what value does it serve if you query each time? > Therefore, query ifname during each devlink port print. > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > --- > devlink/devlink.c | 46 +++++++++++++++++++++++++++++++--------------- > 1 file changed, 31 insertions(+), 15 deletions(-) > > diff --git a/devlink/devlink.c b/devlink/devlink.c > index 8aefa101b2f8..680936f891cf 100644 > --- a/devlink/devlink.c > +++ b/devlink/devlink.c > @@ -864,21 +864,38 @@ static int ifname_map_lookup(struct dl *dl, const char *ifname, > return -ENOENT; > } > > -static int ifname_map_rev_lookup(struct dl *dl, const char *bus_name, > - const char *dev_name, uint32_t port_index, > - char **p_ifname) > +static int port_ifname_get_cb(const struct nlmsghdr *nlh, void *data) > { > - struct ifname_map *ifname_map; > + struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh); > + struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {}; > + char **p_ifname = data; > + const char *ifname; > > - list_for_each_entry(ifname_map, &dl->ifname_map_list, list) { > - if (strcmp(bus_name, ifname_map->bus_name) == 0 && > - strcmp(dev_name, ifname_map->dev_name) == 0 && > - port_index == ifname_map->port_index) { > - *p_ifname = ifname_map->ifname; > - return 0; > - } > - } > - return -ENOENT; > + mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb); > + if (!tb[DEVLINK_ATTR_PORT_NETDEV_NAME]) > + return MNL_CB_ERROR; > + > + ifname = mnl_attr_get_str(tb[DEVLINK_ATTR_PORT_NETDEV_NAME]); > + *p_ifname = strdup(ifname); > + if (!*p_ifname) > + return MNL_CB_ERROR; > + > + return MNL_CB_OK; > +} > + > +static int port_ifname_get(struct dl *dl, const char *bus_name, > + const char *dev_name, uint32_t port_index, > + char **p_ifname) > +{ > + struct nlmsghdr *nlh; > + > + nlh = mnlu_gen_socket_cmd_prepare(&dl->nlg, DEVLINK_CMD_PORT_GET, > + NLM_F_REQUEST | NLM_F_ACK); > + mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, bus_name); > + mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, dev_name); > + mnl_attr_put_u32(nlh, DEVLINK_ATTR_PORT_INDEX, port_index); > + return mnlu_gen_socket_sndrcv(&dl->nlg, nlh, port_ifname_get_cb, > + p_ifname); > } > > static int strtobool(const char *str, bool *p_val) > @@ -2577,8 +2594,7 @@ static void __pr_out_port_handle_start(struct dl *dl, const char *bus_name, > char *ifname = NULL; > > if (dl->no_nice_names || !try_nice || > - ifname_map_rev_lookup(dl, bus_name, dev_name, > - port_index, &ifname) != 0) > + port_ifname_get(dl, bus_name, dev_name, port_index, &ifname) != 0) > sprintf(buf, "%s/%s/%d", bus_name, dev_name, port_index); > else > sprintf(buf, "%s", ifname);
Mon, Nov 07, 2022 at 04:16:42PM CET, dsahern@gmail.com wrote: >On 11/4/22 4:23 AM, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@nvidia.com> >> >> ifname map is created once during init. However, ifnames can easily >> change during the devlink process runtime (e. g. devlink mon). > >why not update the cache on name changes? Doing a query on print has We would have to listen on RTNetlink for the changes, as devlink does not send such events on netdev ifname change. >extra overhead. And, if you insist a per-print query is needed, why >leave ifname_map_list? what value does it serve if you query each time? Correct. > > >> Therefore, query ifname during each devlink port print. >> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com> >> --- >> devlink/devlink.c | 46 +++++++++++++++++++++++++++++++--------------- >> 1 file changed, 31 insertions(+), 15 deletions(-) >> >> diff --git a/devlink/devlink.c b/devlink/devlink.c >> index 8aefa101b2f8..680936f891cf 100644 >> --- a/devlink/devlink.c >> +++ b/devlink/devlink.c >> @@ -864,21 +864,38 @@ static int ifname_map_lookup(struct dl *dl, const char *ifname, >> return -ENOENT; >> } >> >> -static int ifname_map_rev_lookup(struct dl *dl, const char *bus_name, >> - const char *dev_name, uint32_t port_index, >> - char **p_ifname) >> +static int port_ifname_get_cb(const struct nlmsghdr *nlh, void *data) >> { >> - struct ifname_map *ifname_map; >> + struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh); >> + struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {}; >> + char **p_ifname = data; >> + const char *ifname; >> >> - list_for_each_entry(ifname_map, &dl->ifname_map_list, list) { >> - if (strcmp(bus_name, ifname_map->bus_name) == 0 && >> - strcmp(dev_name, ifname_map->dev_name) == 0 && >> - port_index == ifname_map->port_index) { >> - *p_ifname = ifname_map->ifname; >> - return 0; >> - } >> - } >> - return -ENOENT; >> + mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb); >> + if (!tb[DEVLINK_ATTR_PORT_NETDEV_NAME]) >> + return MNL_CB_ERROR; >> + >> + ifname = mnl_attr_get_str(tb[DEVLINK_ATTR_PORT_NETDEV_NAME]); >> + *p_ifname = strdup(ifname); >> + if (!*p_ifname) >> + return MNL_CB_ERROR; >> + >> + return MNL_CB_OK; >> +} >> + >> +static int port_ifname_get(struct dl *dl, const char *bus_name, >> + const char *dev_name, uint32_t port_index, >> + char **p_ifname) >> +{ >> + struct nlmsghdr *nlh; >> + >> + nlh = mnlu_gen_socket_cmd_prepare(&dl->nlg, DEVLINK_CMD_PORT_GET, >> + NLM_F_REQUEST | NLM_F_ACK); >> + mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, bus_name); >> + mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, dev_name); >> + mnl_attr_put_u32(nlh, DEVLINK_ATTR_PORT_INDEX, port_index); >> + return mnlu_gen_socket_sndrcv(&dl->nlg, nlh, port_ifname_get_cb, >> + p_ifname); >> } >> >> static int strtobool(const char *str, bool *p_val) >> @@ -2577,8 +2594,7 @@ static void __pr_out_port_handle_start(struct dl *dl, const char *bus_name, >> char *ifname = NULL; >> >> if (dl->no_nice_names || !try_nice || >> - ifname_map_rev_lookup(dl, bus_name, dev_name, >> - port_index, &ifname) != 0) >> + port_ifname_get(dl, bus_name, dev_name, port_index, &ifname) != 0) >> sprintf(buf, "%s/%s/%d", bus_name, dev_name, port_index); >> else >> sprintf(buf, "%s", ifname); >
On 11/7/22 8:54 AM, Jiri Pirko wrote: > Mon, Nov 07, 2022 at 04:16:42PM CET, dsahern@gmail.com wrote: >> On 11/4/22 4:23 AM, Jiri Pirko wrote: >>> From: Jiri Pirko <jiri@nvidia.com> >>> >>> ifname map is created once during init. However, ifnames can easily >>> change during the devlink process runtime (e. g. devlink mon). >> >> why not update the cache on name changes? Doing a query on print has > > We would have to listen on RTNetlink for the changes, as devlink does > not send such events on netdev ifname change. > > >> extra overhead. And, if you insist a per-print query is needed, why >> leave ifname_map_list? what value does it serve if you query each time? > > Correct. "Correct" is not a response to a series of questions. You followed up that 1 word response with a 2 patch set that has the same title as patch 3 in this set. Please elaborate.
Tue, Nov 08, 2022 at 04:59:23PM CET, dsahern@gmail.com wrote: >On 11/7/22 8:54 AM, Jiri Pirko wrote: >> Mon, Nov 07, 2022 at 04:16:42PM CET, dsahern@gmail.com wrote: >>> On 11/4/22 4:23 AM, Jiri Pirko wrote: >>>> From: Jiri Pirko <jiri@nvidia.com> >>>> >>>> ifname map is created once during init. However, ifnames can easily >>>> change during the devlink process runtime (e. g. devlink mon). >>> >>> why not update the cache on name changes? Doing a query on print has >> >> We would have to listen on RTNetlink for the changes, as devlink does >> not send such events on netdev ifname change. >> >> >>> extra overhead. And, if you insist a per-print query is needed, why >>> leave ifname_map_list? what value does it serve if you query each time? >> >> Correct. > >"Correct" is not a response to a series of questions. > >You followed up that 1 word response with a 2 patch set that has the >same title as patch 3 in this set. Please elaborate. Sorry. You are correct, per-print query makes the map needless. That is what I ment. I'm redoing this, tracking the changes coming from the kernel (port ifname change actually gets propagated). I sent v2 without this patch included, that means it contains only patch 3 with the dependency patch. That is why the patchset has the same name as patch 3. I'm working on the tracking patchset to replace this patch. Hope this cleared your concerns. Thanks!
diff --git a/devlink/devlink.c b/devlink/devlink.c index 8aefa101b2f8..680936f891cf 100644 --- a/devlink/devlink.c +++ b/devlink/devlink.c @@ -864,21 +864,38 @@ static int ifname_map_lookup(struct dl *dl, const char *ifname, return -ENOENT; } -static int ifname_map_rev_lookup(struct dl *dl, const char *bus_name, - const char *dev_name, uint32_t port_index, - char **p_ifname) +static int port_ifname_get_cb(const struct nlmsghdr *nlh, void *data) { - struct ifname_map *ifname_map; + struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh); + struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {}; + char **p_ifname = data; + const char *ifname; - list_for_each_entry(ifname_map, &dl->ifname_map_list, list) { - if (strcmp(bus_name, ifname_map->bus_name) == 0 && - strcmp(dev_name, ifname_map->dev_name) == 0 && - port_index == ifname_map->port_index) { - *p_ifname = ifname_map->ifname; - return 0; - } - } - return -ENOENT; + mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb); + if (!tb[DEVLINK_ATTR_PORT_NETDEV_NAME]) + return MNL_CB_ERROR; + + ifname = mnl_attr_get_str(tb[DEVLINK_ATTR_PORT_NETDEV_NAME]); + *p_ifname = strdup(ifname); + if (!*p_ifname) + return MNL_CB_ERROR; + + return MNL_CB_OK; +} + +static int port_ifname_get(struct dl *dl, const char *bus_name, + const char *dev_name, uint32_t port_index, + char **p_ifname) +{ + struct nlmsghdr *nlh; + + nlh = mnlu_gen_socket_cmd_prepare(&dl->nlg, DEVLINK_CMD_PORT_GET, + NLM_F_REQUEST | NLM_F_ACK); + mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, bus_name); + mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, dev_name); + mnl_attr_put_u32(nlh, DEVLINK_ATTR_PORT_INDEX, port_index); + return mnlu_gen_socket_sndrcv(&dl->nlg, nlh, port_ifname_get_cb, + p_ifname); } static int strtobool(const char *str, bool *p_val) @@ -2577,8 +2594,7 @@ static void __pr_out_port_handle_start(struct dl *dl, const char *bus_name, char *ifname = NULL; if (dl->no_nice_names || !try_nice || - ifname_map_rev_lookup(dl, bus_name, dev_name, - port_index, &ifname) != 0) + port_ifname_get(dl, bus_name, dev_name, port_index, &ifname) != 0) sprintf(buf, "%s/%s/%d", bus_name, dev_name, port_index); else sprintf(buf, "%s", ifname);