netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [patch iproute2-next 0/3] devlink: make ifname-port relationship behaviour better
@ 2022-11-04 10:23 Jiri Pirko
  2022-11-04 10:23 ` [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup Jiri Pirko
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Jiri Pirko @ 2022-11-04 10:23 UTC (permalink / raw)
  To: netdev; +Cc: sthemmin, dsahern, kuba, moshe, aeedm

From: Jiri Pirko <jiri@nvidia.com>

This patchset consists of 2 items:
1) patch #1 is fixing output of "devlink mon port" by considering
   possible ifname change during run.
2) patch #3 is benefiting of newly added IFLA_DEVLINK_PORT attribute
   exposing netdev link to devlink port directly. Patch #2 is just
   dependency for patch #3

Jiri Pirko (3):
  devlink: query ifname for devlink port instead of map lookup
  devlink: add ifname_map_add/del() helpers
  devlink: get devlink port for ifname using RTNL get link command

 devlink/devlink.c | 164 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 140 insertions(+), 24 deletions(-)

-- 
2.37.3


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup
  2022-11-04 10:23 [patch iproute2-next 0/3] devlink: make ifname-port relationship behaviour better Jiri Pirko
@ 2022-11-04 10:23 ` Jiri Pirko
  2022-11-07 15:16   ` David Ahern
  2022-11-04 10:23 ` [patch iproute2-next 2/3] devlink: add ifname_map_add/del() helpers Jiri Pirko
  2022-11-04 10:23 ` [patch iproute2-next 3/3] devlink: get devlink port for ifname using RTNL get link command Jiri Pirko
  2 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2022-11-04 10:23 UTC (permalink / raw)
  To: netdev; +Cc: sthemmin, dsahern, kuba, moshe, aeedm

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).
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);
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [patch iproute2-next 2/3] devlink: add ifname_map_add/del() helpers
  2022-11-04 10:23 [patch iproute2-next 0/3] devlink: make ifname-port relationship behaviour better Jiri Pirko
  2022-11-04 10:23 ` [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup Jiri Pirko
@ 2022-11-04 10:23 ` Jiri Pirko
  2022-11-04 10:23 ` [patch iproute2-next 3/3] devlink: get devlink port for ifname using RTNL get link command Jiri Pirko
  2 siblings, 0 replies; 8+ messages in thread
From: Jiri Pirko @ 2022-11-04 10:23 UTC (permalink / raw)
  To: netdev; +Cc: sthemmin, dsahern, kuba, moshe, aeedm

From: Jiri Pirko <jiri@nvidia.com>

Add couple of helpers to alloc/free of map object alongside with list
addition/removal.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 devlink/devlink.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 680936f891cf..6e8e03aa14b7 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -774,16 +774,35 @@ static int function_attr_cb(const struct nlattr *attr, void *data)
 	return MNL_CB_OK;
 }
 
+static int ifname_map_add(struct dl *dl, const char *ifname,
+			  const char *bus_name, const char *dev_name,
+			  uint32_t port_index)
+{
+	struct ifname_map *ifname_map;
+
+	ifname_map = ifname_map_alloc(bus_name, dev_name, port_index, ifname);
+	if (!ifname_map)
+		return -ENOMEM;
+	list_add(&ifname_map->list, &dl->ifname_map_list);
+	return 0;
+}
+
+static void ifname_map_del(struct ifname_map *ifname_map)
+{
+	list_del(&ifname_map->list);
+	ifname_map_free(ifname_map);
+}
+
 static int ifname_map_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
 	struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
 	struct dl *dl = data;
-	struct ifname_map *ifname_map;
 	const char *bus_name;
 	const char *dev_name;
 	uint32_t port_index;
 	const char *port_ifname;
+	int err;
 
 	mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb);
 	if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME] ||
@@ -797,11 +816,9 @@ static int ifname_map_cb(const struct nlmsghdr *nlh, void *data)
 	dev_name = mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]);
 	port_index = mnl_attr_get_u32(tb[DEVLINK_ATTR_PORT_INDEX]);
 	port_ifname = mnl_attr_get_str(tb[DEVLINK_ATTR_PORT_NETDEV_NAME]);
-	ifname_map = ifname_map_alloc(bus_name, dev_name,
-				      port_index, port_ifname);
-	if (!ifname_map)
+	err = ifname_map_add(dl, port_ifname, bus_name, dev_name, port_index);
+	if (err)
 		return MNL_CB_ERROR;
-	list_add(&ifname_map->list, &dl->ifname_map_list);
 
 	return MNL_CB_OK;
 }
@@ -812,8 +829,7 @@ static void ifname_map_fini(struct dl *dl)
 
 	list_for_each_entry_safe(ifname_map, tmp,
 				 &dl->ifname_map_list, list) {
-		list_del(&ifname_map->list);
-		ifname_map_free(ifname_map);
+		ifname_map_del(ifname_map);
 	}
 }
 
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [patch iproute2-next 3/3] devlink: get devlink port for ifname using RTNL get link command
  2022-11-04 10:23 [patch iproute2-next 0/3] devlink: make ifname-port relationship behaviour better Jiri Pirko
  2022-11-04 10:23 ` [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup Jiri Pirko
  2022-11-04 10:23 ` [patch iproute2-next 2/3] devlink: add ifname_map_add/del() helpers Jiri Pirko
@ 2022-11-04 10:23 ` Jiri Pirko
  2 siblings, 0 replies; 8+ messages in thread
From: Jiri Pirko @ 2022-11-04 10:23 UTC (permalink / raw)
  To: netdev; +Cc: sthemmin, dsahern, kuba, moshe, aeedm

From: Jiri Pirko <jiri@nvidia.com>

Currently, when user specifies ifname as a handle on command line of
devlink, the related devlink port is looked-up in previously taken dump
of all devlink ports on the system. There are 3 problems with that:
1) The dump iterates over all devlink instances in kernel and takes a
   devlink instance lock for each.
2) Dumping all devlink ports would not scale.
3) Alternative ifnames are not exposed by devlink netlink interface.

Instead, benefit from RTNL get link command extension and get the
devlink port handle info from IFLA_DEVLINK_PORT attribute, if supported.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 devlink/devlink.c | 88 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 86 insertions(+), 2 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 6e8e03aa14b7..f656d0a7c514 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -43,6 +43,8 @@
 #include "json_print.h"
 #include "utils.h"
 #include "namespace.h"
+#include "libnetlink.h"
+#include "../ip/ip_common.h"
 
 #define ESWITCH_MODE_LEGACY "legacy"
 #define ESWITCH_MODE_SWITCHDEV "switchdev"
@@ -793,6 +795,81 @@ static void ifname_map_del(struct ifname_map *ifname_map)
 	ifname_map_free(ifname_map);
 }
 
+static int ifname_map_rtnl_port_parse(struct dl *dl, const char *ifname,
+				      struct rtattr *nest)
+{
+	struct rtattr *tb[DEVLINK_ATTR_MAX + 1];
+	const char *bus_name;
+	const char *dev_name;
+	uint32_t port_index;
+
+	parse_rtattr_nested(tb, DEVLINK_ATTR_MAX, nest);
+	if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME] ||
+	    !tb[DEVLINK_ATTR_PORT_INDEX])
+		return -ENOENT;
+
+	bus_name = rta_getattr_str(tb[DEVLINK_ATTR_BUS_NAME]);
+	dev_name = rta_getattr_str(tb[DEVLINK_ATTR_DEV_NAME]);
+	port_index = rta_getattr_u32(tb[DEVLINK_ATTR_PORT_INDEX]);
+	return ifname_map_add(dl, ifname, bus_name, dev_name, port_index);
+}
+
+static int ifname_map_rtnl_init(struct dl *dl, const char *ifname)
+{
+	struct iplink_req req = {
+		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
+		.n.nlmsg_flags = NLM_F_REQUEST,
+		.n.nlmsg_type = RTM_GETLINK,
+		.i.ifi_family = AF_UNSPEC,
+	};
+	struct rtattr *tb[IFLA_MAX + 1];
+	struct rtnl_handle rth;
+	struct ifinfomsg *ifi;
+	struct nlmsghdr *n;
+	int len;
+	int err;
+
+	if (rtnl_open(&rth, 0) < 0) {
+		pr_err("Cannot open rtnetlink\n");
+		return -EINVAL;
+	}
+
+	addattr_l(&req.n, sizeof(req),
+		  !check_ifname(ifname) ? IFLA_IFNAME : IFLA_ALT_IFNAME,
+		  ifname, strlen(ifname) + 1);
+
+	if (rtnl_talk(&rth, &req.n, &n) < 0) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	if (n->nlmsg_type != RTM_NEWLINK) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	ifi = NLMSG_DATA(n);
+	len = n->nlmsg_len;
+
+	len -= NLMSG_LENGTH(sizeof(*ifi));
+	if (len < 0) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	parse_rtattr_flags(tb, IFLA_MAX, IFLA_RTA(ifi), len, NLA_F_NESTED);
+	if (!tb[IFLA_DEVLINK_PORT]) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	err = ifname_map_rtnl_port_parse(dl, ifname, tb[IFLA_DEVLINK_PORT]);
+
+out:
+	rtnl_close(&rth);
+	return err;
+}
+
 static int ifname_map_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
@@ -838,11 +915,18 @@ static void ifname_map_init(struct dl *dl)
 	INIT_LIST_HEAD(&dl->ifname_map_list);
 }
 
-static int ifname_map_load(struct dl *dl)
+static int ifname_map_load(struct dl *dl, const char *ifname)
 {
 	struct nlmsghdr *nlh;
 	int err;
 
+	err = ifname_map_rtnl_init(dl, ifname);
+	if (!err)
+		return 0;
+	/* In case kernel does not support devlink port info passed over
+	 * RT netlink, fall-back to ports dump.
+	 */
+
 	nlh = mnlu_gen_socket_cmd_prepare(&dl->nlg, DEVLINK_CMD_PORT_GET,
 			       NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP);
 
@@ -862,7 +946,7 @@ static int ifname_map_lookup(struct dl *dl, const char *ifname,
 	int err;
 
 	if (!dl->map_loaded) {
-		err = ifname_map_load(dl);
+		err = ifname_map_load(dl, ifname);
 		if (err) {
 			pr_err("Failed to create index map\n");
 			return err;
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup
  2022-11-04 10:23 ` [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup Jiri Pirko
@ 2022-11-07 15:16   ` David Ahern
  2022-11-07 15:54     ` Jiri Pirko
  0 siblings, 1 reply; 8+ messages in thread
From: David Ahern @ 2022-11-07 15:16 UTC (permalink / raw)
  To: Jiri Pirko, netdev; +Cc: sthemmin, kuba, moshe, aeedm

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);


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup
  2022-11-07 15:16   ` David Ahern
@ 2022-11-07 15:54     ` Jiri Pirko
  2022-11-08 15:59       ` David Ahern
  0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2022-11-07 15:54 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, sthemmin, kuba, moshe, aeedm

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);
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup
  2022-11-07 15:54     ` Jiri Pirko
@ 2022-11-08 15:59       ` David Ahern
  2022-11-09 11:57         ` Jiri Pirko
  0 siblings, 1 reply; 8+ messages in thread
From: David Ahern @ 2022-11-08 15:59 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: netdev, kuba, moshe

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup
  2022-11-08 15:59       ` David Ahern
@ 2022-11-09 11:57         ` Jiri Pirko
  0 siblings, 0 replies; 8+ messages in thread
From: Jiri Pirko @ 2022-11-09 11:57 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, kuba, moshe

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!

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-11-09 11:57 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-04 10:23 [patch iproute2-next 0/3] devlink: make ifname-port relationship behaviour better Jiri Pirko
2022-11-04 10:23 ` [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup Jiri Pirko
2022-11-07 15:16   ` David Ahern
2022-11-07 15:54     ` Jiri Pirko
2022-11-08 15:59       ` David Ahern
2022-11-09 11:57         ` Jiri Pirko
2022-11-04 10:23 ` [patch iproute2-next 2/3] devlink: add ifname_map_add/del() helpers Jiri Pirko
2022-11-04 10:23 ` [patch iproute2-next 3/3] devlink: get devlink port for ifname using RTNL get link command Jiri Pirko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).