Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces

[David Ahern <dsahern@gmail.com>]

On 8/5/19 9:20 AM, Jiri Pirko wrote:
> Mon, Aug 05, 2019 at 04:51:22PM CEST, dsahern@gmail.com wrote:
>> On 8/5/19 8:49 AM, Jiri Pirko wrote:
>>>> Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>>>> per-namepace accounting to all namespaces managed by a single devlink
>>>> instance in init_net - which is completely wrong.
>>> No. Not "all namespaces". Only the one where the devlink is. And that is
>>> always init_net, until this patchset.
>>>
>>>
>>
>> Jiri: your change to fib.c does not take into account namespace when
>> doing rules and routes accounting. you broke it. fix it.
> 
> What do you mean by "account namespace"? It's a device resource, why to
> tight it with namespace? What if you have 2 netdevsim-devlink instances
> in one namespace? Why the setting should be per-namespace?
> 

Jiri:

Here's an example of how your 5.2 change to netdevsim broke the resource
controller:

Create a netdevsim device:
$ modprobe netdevsim
$  echo "0 1" > /sys/bus/netdevsim/new_device

Get the current number of IPv4 routes:
$ n=$(ip -4 ro ls table all | wc -l)
$ echo $n
13

Prevent any more from being added. This limit should apply solely to
this namespace, init_net:

$ devlink resource set netdevsim/netdevsim0 path /IPv4/fib size $n
$ devlink dev reload netdevsim/netdevsim0
Error: netdevsim: New size is less than current occupancy.
devlink answers: Invalid argument

So that is the first breakage: accounting is off - maybe. Note there are
no other visible namespaces, but who knows what systemd or other
processes are doing with namespaces. Perhaps this accounting is another
example of your changes not properly handling namespaces:

$ devlink resource show netdevsim/netdevsim0
netdevsim/netdevsim0:
  name IPv4 size unlimited unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
    resources:
      name fib size 13 occ 17 unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
      name fib-rules size unlimited occ 6 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none
  name IPv6 size unlimited unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
    resources:
      name fib size unlimited occ 10 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none
      name fib-rules size unlimited occ 4 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none

So the occupancy does not match the tables for init_net.

Reset the max to 17, the current occupancy:
$ devlink resource set netdevsim/netdevsim0 path /IPv4/fib size 17
$ devlink dev reload netdevsim/netdevsim0
$ devlink resource show netdevsim/netdevsim0
netdevsim/netdevsim0:
  name IPv4 size unlimited unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
    resources:
      name fib size 17 occ 17 unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
      name fib-rules size unlimited occ 6 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none
  name IPv6 size unlimited unit entry size_min 0 size_max unlimited
size_gran 1 dpipe_tables none
    resources:
      name fib size unlimited occ 10 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none
      name fib-rules size unlimited occ 4 unit entry size_min 0 size_max
unlimited size_gran 1 dpipe_tables none

Create a new namespace, bring up lo which attempts to add more route
entries:
$ unshare -n
$ ip li set lo up

If you list routes you see the lo routes failed to installed because of
the limits, but it is a silent failure. Try to add a new route and you
see the cross namespace accounting now:
$ ip ro add 192.168.1.0/24 dev lo
Error: netdevsim: Exceeded number of supported fib entries.


Contrast that behavior with 5.1 and you see the new namespaces have no
bearing on accounting in init_net and limits in init_net do not affect
other namespaces.

That behavior needs to be restored in 5.2 and 5.3.