Re: [patch iproute2-next 1/3] devlink: query ifname for devlink port instead of map lookup

[Jiri Pirko <jiri@resnulli.us>]

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