Message-ID: <20211128111313.hjywmtmnipg4ul4f@kgollan-pc>
Date:   Sun, 28 Nov 2021 13:13:14 +0200
From:   Lahav Schlesinger <lschlesinger@...venets.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, dsahern@...il.com
Subject: Re: [PATCH net-next v3] rtnetlink: Support fine-grained netdevice
 bulk deletion

On Sun, Nov 28, 2021 at 09:33:01AM +0200, Leon Romanovsky wrote:
> CAUTION: External E-Mail - Use caution with links and attachments
>
>
> On Thu, Nov 25, 2021 at 06:51:46PM +0200, Lahav Schlesinger wrote:
> > Under large scale, some routers are required to support tens of thousands
> > of devices at once, both physical and virtual (e.g. loopbacks, tunnels,
> > vrfs, etc).
> > At times such routers are required to delete massive amounts of devices
> > at once, such as when a factory reset is performed on the router (causing
> > a deletion of all devices), or when a configuration is restored after an
> > upgrade, or as a request from an operator.
> >
> > Currently there are 2 means of deleting devices using Netlink:
> > 1. Deleting a single device (either by ifindex using ifinfomsg::ifi_index,
> > or by name using IFLA_IFNAME)
> > 2. Delete all device that belong to a group (using IFLA_GROUP)
> >
> > Deletion of devices one-by-one has poor performance on large scale of
> > devices compared to "group deletion":
> > After all device are handled, netdev_run_todo() is called which
> > calls rcu_barrier() to finish any outstanding RCU callbacks that were
> > registered during the deletion of the device, then wait until the
> > refcount of all the devices is 0, then perform final cleanups.
> >
> > However, calling rcu_barrier() is a very costly operation, each call
> > taking in the order of 10s of milliseconds.
> >
> > When deleting a large number of device one-by-one, rcu_barrier()
> > will be called for each device being deleted.
> > As an example, following benchmark deletes 10K loopback devices,
> > all of which are UP and with only IPv6 LLA being configured:
> >
> > 1. Deleting one-by-one using 1 thread : 243 seconds
> > 2. Deleting one-by-one using 10 thread: 70 seconds
> > 3. Deleting one-by-one using 50 thread: 54 seconds
> > 4. Deleting all using "group deletion": 30 seconds
> >
> > Note that even though the deletion logic takes place under the rtnl
> > lock, since the call to rcu_barrier() is outside the lock we gain
> > some improvements.
> >
> > But, while "group deletion" is the fastest, it is not suited for
> > deleting large number of arbitrary devices which are unknown a head of
> > time. Furthermore, moving large number of devices to a group is also a
> > costly operation.
> >
> > This patch adds support for passing an arbitrary list of ifindex of
> > devices to delete with a new IFLA_IFINDEX_LIST attribute.
> > This gives a more fine-grained control over which devices to delete,
> > while still resulting in rcu_barrier() being called only once.
> > Indeed, the timings of using this new API to delete 10K devices is
> > the same as using the existing "group" deletion.
> >
> > The size constraints on the attribute means the API can delete at most
> > 16382 devices in a single request.
> >
> > Signed-off-by: Lahav Schlesinger <lschlesinger@...venets.com>
> > ---
> > v2 -> v3
> >  - Rename 'ifindex_list' to 'ifindices', and pass it as int*
> >  - Clamp 'ops' variable in second loop.
> >
> > v1 -> v2
> >  - Unset 'len' of IFLA_IFINDEX_LIST in policy.
> >  - Use __dev_get_by_index() instead of n^2 loop.
> >  - Return -ENODEV if any ifindex is not present.
> >  - Saved devices in an array.
> >  - Fix formatting.
> >
> >  include/uapi/linux/if_link.h |  1 +
> >  net/core/rtnetlink.c         | 50 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 51 insertions(+)
> >
> > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> > index eebd3894fe89..f950bf6ed025 100644
> > --- a/include/uapi/linux/if_link.h
> > +++ b/include/uapi/linux/if_link.h
> > @@ -348,6 +348,7 @@ enum {
> >       IFLA_PARENT_DEV_NAME,
> >       IFLA_PARENT_DEV_BUS_NAME,
> >
> > +     IFLA_IFINDEX_LIST,
> >       __IFLA_MAX
> >  };
> >
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index fd030e02f16d..49d1a3954a01 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1880,6 +1880,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_LIST]     = { .type = NLA_BINARY },
> >  };
> >
> >  static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
> > @@ -3050,6 +3051,52 @@ static int rtnl_group_dellink(const struct net *net, int group)
> >       return 0;
> >  }
> >
> > +static int rtnl_list_dellink(struct net *net, int *ifindices, int size)
> > +{
> > +     const int num_devices = size / sizeof(int);
> > +     struct net_device **dev_list;
> > +     LIST_HEAD(list_kill);
> > +     int i, ret;
> > +
> > +     if (size <= 0 || size % sizeof(int))
> > +             return -EINVAL;
> > +
> > +     dev_list = kmalloc_array(num_devices, sizeof(*dev_list), GFP_KERNEL);
> > +     if (!dev_list)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < num_devices; i++) {
> > +             const struct rtnl_link_ops *ops;
> > +             struct net_device *dev;
> > +
> > +             ret = -ENODEV;
> > +             dev = __dev_get_by_index(net, ifindices[i]);
> > +             if (!dev)
> > +                     goto out_free;
> > +
> > +             ret = -EOPNOTSUPP;
> > +             ops = dev->rtnl_link_ops;
> > +             if (!ops || !ops->dellink)
> > +                     goto out_free;
>
> I'm just curious, how does user know that specific device doesn't
> have ->delink implementation? It is important to know because you
> are failing whole batch deletion. At least for single delink, users
> have chance to skip "failed" one and continue.
>
> Thanks

Hi Leon, I don't see any immediate way users can get this information.
I do think that failing the whole request is better than silently
ignoring such devices.

Perhaps an alternative is to return the unsupported device's name in an
extack? To make NL_SET_ERR_MSG() support string formatting this will
require changing netlink_ext_ack::_msg to be an array though (skimming
over the calls to NL_SET_ERR_MSG(), a buffer of size say 128 should be
large enough).

>
> > +
> > +             dev_list[i] = dev;
> > +     }
> > +
> > +     for (i = 0; i < num_devices; i++) {
> > +             struct net_device *dev = dev_list[i];
> > +
> > +             dev->rtnl_link_ops->dellink(dev, &list_kill);
> > +     }
> > +
> > +     unregister_netdevice_many(&list_kill);
> > +
> > +     ret = 0;
> > +
> > +out_free:
> > +     kfree(dev_list);
> > +     return ret;
> > +}
> > +
> >  int rtnl_delete_link(struct net_device *dev)
> >  {
> >       const struct rtnl_link_ops *ops;
> > @@ -3102,6 +3149,9 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
> >                                  tb[IFLA_ALT_IFNAME], NULL);
> >       else if (tb[IFLA_GROUP])
> >               err = rtnl_group_dellink(tgt_net, nla_get_u32(tb[IFLA_GROUP]));
> > +     else if (tb[IFLA_IFINDEX_LIST])
> > +             err = rtnl_list_dellink(tgt_net, nla_data(tb[IFLA_IFINDEX_LIST]),
> > +                                     nla_len(tb[IFLA_IFINDEX_LIST]));
> >       else
> >               goto out;
> >
> > --
> > 2.25.1
> >

Powered by blists - more mailing lists