netdev - Re: [patch net-next v2 15/16] netdevsim: move netdev creation/destruction to dev probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190423191415.GK2677@nanopsycho.orion>
Date:   Tue, 23 Apr 2019 21:14:15 +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

Tue, Apr 23, 2019 at 07:05:28PM CEST, jakub.kicinski@...ronome.com wrote:
>On Tue, 23 Apr 2019 09:20:14 +0200, Jiri Pirko wrote:
>> 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.
>
>Technically I have no opinion, personally I find it petty and
>unnecessary in the git era..  Probably not worth your time now 
>to go and remove :)
>
>> >> +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.
>
>Mm.. true.  I must say I had a ton of issues with this in Netronome
>test systems, those "wait for renames" loop always broke no matter
>what..  OS upgrade, adding debug stuff into the kernel..  I get
>flashbacks.  I've seen udev easily take 20s to rename on a heavy kernel.
>Since we replaced the loops with udevadm settle - it's been a bliss.  
>YMMV but in my experience regardless of workqueues and all udev is way
>more reliable.

Sure, But I don't see how udevadm settle would help here. Hmm but
perhaps I can poll sysfs until the device appears there and then I can
udevadm settle.


>
>> >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, .."
>
>That is not great :(  The kbuild bot runs BPF offload tests, they run
>with some CentOS user space.  If we don't want to name the device in
>the kernel, perhaps it'd be possible to scan sysfs?  The ifc names
>should be under $device/net/, right?

Yeah, hmm. I'll think about it and fix. Thanks!