Re: [PATCH net-next v5] rtnetlink: Support fine-grained netdevice bulk deletion

[David Ahern <dsahern@gmail.com>]

On 12/5/21 2:36 AM, Lahav Schlesinger wrote:
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index fd030e02f16d..5165cc699d97 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -37,6 +37,7 @@
>  #include <linux/pci.h>
>  #include <linux/etherdevice.h>
>  #include <linux/bpf.h>
> +#include <linux/sort.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -1880,6 +1881,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
>  	[IFLA_PROTO_DOWN_REASON] = { .type = NLA_NESTED },
>  	[IFLA_NEW_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
>  	[IFLA_PARENT_DEV_NAME]	= { .type = NLA_NUL_STRING },
> +	[IFLA_IFINDEX]		= { .type = NLA_S32 },

you need to make sure this new attribute can not be used in setlink
requests or getlink.

>  };
>  
>  static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
> @@ -3050,6 +3052,78 @@ static int rtnl_group_dellink(const struct net *net, int group)
>  	return 0;
>  }
>  
> +static int dev_ifindex_cmp(const void *a, const void *b)
> +{
> +	struct net_device * const *dev1 = a, * const *dev2 = b;

const struct net_device *dev1 = 1, *dev2 = b;

> +
> +	return (*dev1)->ifindex - (*dev2)->ifindex;
> +}
> +
> +static int rtnl_ifindex_dellink(struct net *net, struct nlattr *head, int len,
> +				struct netlink_ext_ack *extack)
> +{
> +	int i = 0, num_devices = 0, rem;
> +	struct net_device **dev_list;
> +	const struct nlattr *nla;
> +	LIST_HEAD(list_kill);
> +	int ret;
> +
> +	nla_for_each_attr(nla, head, len, rem) {
> +		if (nla_type(nla) == IFLA_IFINDEX)
> +			num_devices++;
> +	}


The need to walk the list twice (3 really with the sort) to means the
array solution is better.

> +
> +	dev_list = kmalloc_array(num_devices, sizeof(*dev_list), GFP_KERNEL);
> +	if (!dev_list)
> +		return -ENOMEM;
> +
> +	nla_for_each_attr(nla, head, len, rem) {
> +		const struct rtnl_link_ops *ops;
> +		struct net_device *dev;
> +		int ifindex;
> +
> +		if (nla_type(nla) != IFLA_IFINDEX)
> +			continue;

and this business too. We have arrays in other places
(net/ipv4/nexthop.c), so this is not the first.


> +
> +		ifindex = nla_get_s32(nla);
> +		ret = -ENODEV;
> +		dev = __dev_get_by_index(net, ifindex);
> +		if (!dev) {
> +			NL_SET_ERR_MSG_ATTR(extack, nla, "Unknown ifindex");
> +			goto out_free;
> +		}
> +
> +		ret = -EOPNOTSUPP;
> +		ops = dev->rtnl_link_ops;
> +		if (!ops || !ops->dellink) {
> +			NL_SET_ERR_MSG_ATTR(extack, nla, "Device cannot be deleted");
> +			goto out_free;
> +		}
> +
> +		dev_list[i++] = dev;
> +	}
> +
> +	/* Sort devices, so we could skip duplicates */
> +	sort(dev_list, num_devices, sizeof(*dev_list), dev_ifindex_cmp, NULL);

how did this sort change the results? 10k compares and re-order has to
add some overhead.

> +
> +	for (i = 0; i < num_devices; i++) {
> +		struct net_device *dev = dev_list[i];
> +
> +		if (i != 0 && dev_list[i - 1]->ifindex == dev->ifindex)

		if (i && ...)


I liked the array variant better. Jakub?