Message-ID: <20190423072014.GC2677@nanopsycho.orion>
Date:   Tue, 23 Apr 2019 09:20:14 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, mlxsw@...lanox.com,
        dsahern@...il.com
Subject: Re: [patch net-next v2 15/16] netdevsim: move netdev
 creation/destruction to dev probe

Mon, Apr 22, 2019 at 09:31:33PM CEST, jakub.kicinski@...ronome.com wrote:
>On Sat, 20 Apr 2019 12:29:21 +0200, Jiri Pirko wrote:
>> diff --git a/tools/testing/selftests/bpf/test_offload.py b/tools/testing/selftests/bpf/test_offload.py
>> index 5f2e4f9e70e4..a0566dcf064a 100755
>> --- a/tools/testing/selftests/bpf/test_offload.py
>> +++ b/tools/testing/selftests/bpf/test_offload.py
>> @@ -1,6 +1,7 @@
>>  #!/usr/bin/python3
>>  
>>  # Copyright (C) 2017 Netronome Systems, Inc.
>> +# Copyright (c) 2019 Mellanox Technologies. All rights reserved
>
>What's your guiding principle with adding those copyright lines
>everywhere?

Semi-random :) I can remove it if you want.


>
>>  # This software is licensed under the GNU General License Version 2,
>>  # June 1991 as shown in the file COPYING in the top-level directory of this
>> @@ -24,6 +25,7 @@ import struct
>>  import subprocess
>>  import time
>>  import traceback
>> +import errno
>
>aphabetic order.

Ok.


>
>>  
>>  logfile = None
>>  log_level = 1
>> @@ -323,42 +325,85 @@ class DebugfsDir:
>>  
>>          return dfs
>>  
>> -class NetdevSim:
>> +class NetdevSimDev:
>>      """
>> -    Class for netdevsim netdevice and its attributes.
>> +    Class for netdevsim bus device and its attributes.
>>      """
>>  
>> -    def __init__(self, link=None):
>> -        self.link = link
>> +    def __init__(self, port_count=1):
>> +        addr = 0
>> +        while True:
>> +            try:
>> +                with open("/sys/bus/netdevsim/new_device","w") as f:
>
>space after ,

Ok.


>
>> +                    f.write("%u %u" % (addr, port_count))
>> +            except OSError as e:
>> +                if e.errno == errno.ENOSPC:
>> +                    addr += 1
>> +                    continue
>> +                raise e
>> +            break
>>  
>> -        self.dev = self._netdevsim_create()
>>          devs.append(self)
>> +        self.addr = addr
>> +        self.dfs_dir = "/sys/kernel/debug/netdevsim/netdevsim%u/" % addr
>>  
>> -        self.ns = ""
>> +        self.nsims = []
>> +        for port_index in range(port_count):
>> +            self.nsims.append(NetdevSim(self, port_index))
>>  
>> -        self.dfs_dir = '/sys/kernel/debug/netdevsim/netdevsim0/ports/0/'
>> -        self.dev_dir = self.dfs_dir + '/dev/'
>> -        self.dfs_refresh()
>> +    def dfs_num_bound_progs(self):
>> +        path = os.path.join(self.dfs_dir, "bpf_bound_progs")
>> +        _, progs = cmd('ls %s' % (path))
>> +        return len(progs.split())
>>  
>> -    def __getitem__(self, key):
>> -        return self.dev[key]
>> +    def dfs_get_bound_progs(self, expected):
>> +        progs = DebugfsDir(os.path.join(self.dfs_dir, "bpf_bound_progs"))
>> +        if expected is not None:
>> +            if len(progs) != expected:
>> +                fail(True, "%d BPF programs bound, expected %d" %
>> +                     (len(progs), expected))
>> +        return progs
>>  
>> -    def _netdevsim_create(self):
>> -        link = "" if self.link is None else "link " + self.link.dev['ifname']
>> -        _, old  = ip("link show")
>> -        ip("link add sim%d {link} type netdevsim".format(link=link))
>> -        _, new  = ip("link show")
>> +    def remove(self):
>> +        with open("/sys/bus/netdevsim/del_device","w") as f:
>
>and here, etc.

ok


>
>> +            f.write("%u" % self.addr)
>> +        devs.remove(self)
>>  
>> -        for dev in new:
>> -            f = filter(lambda x: x["ifname"] == dev["ifname"], old)
>> -            if len(list(f)) == 0:
>> -                return dev
>> +    def remove_nsim(self, nsim):
>> +        self.nsims.remove(nsim)
>> +        with open("/sys/bus/netdevsim/devices/netdevsim%u/del_port" % self.addr ,"w") as f:
>> +            f.write("%u" % nsim.port_index)
>>  
>> -        raise Exception("failed to create netdevsim device")
>> +class NetdevSim:
>> +    """
>> +    Class for netdevsim netdevice and its attributes.
>> +    """
>> +
>> +    def __init__(self, nsimdev, port_index):
>> +        self.nsimdev = nsimdev
>> +        self.port_index = port_index
>> +        self.ns = ""
>> +        self.dfs_dir = "%s/ports/%u/" % (nsimdev.dfs_dir, port_index)
>> +        self.dfs_refresh()
>> +
>> +        ifname = "eni%unp%u" % (nsimdev.addr, port_index + 1)
>> +        timeout = 0.5
>> +        timeout_start = time.time()
>> +
>> +        while True:
>> +            try:
>> +                _, [self.dev] = ip("link show dev %s" % ifname)
>> +            except Exception as e:
>> +                if time.time() < timeout_start + timeout:
>> +                    continue
>> +                raise e
>> +            break
>
>udevadm settle?  The reliance on latest systemd is a real bad idea,

Not sure that "udevadm settle" would help here. The diver bus probe handle
may be executed from a workqueue, so here, there might be the udev event
queue empty yet still the netdev is not present.


>could you fall back to netdevsimX names if eni$SOME$CUFT is not found?

That would not help. If you have udev which does not support this, you
are going to endup with "eth0, eth1, .."


>
>> +    def __getitem__(self, key):
>> +        return self.dev[key]
>>  
>>      def remove(self):
>> -        devs.remove(self)
>> -        ip("link del dev %s" % (self.dev["ifname"]), ns=self.ns)
>> +        self.nsimdev.remove_nsim(self)
>>  
>>      def dfs_refresh(self):
>>          self.dfs = DebugfsDir(self.dfs_dir)

Powered by blists - more mailing lists