* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Andrii Nakryiko @ 2019-02-27 23:34 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
In-Reply-To: <20190227212256.3856473-1-yhs@fb.com>
On Wed, Feb 27, 2019 at 1:23 PM Yonghong Song <yhs@fb.com> wrote:
>
> Commit d83525ca62cf ("bpf: introduce bpf_spin_lock")
> introduced bpf_spin_lock and the field spin_lock_off
> in kernel internal structure bpf_map has the following
> meaning:
> >=0 valid offset, <0 error
>
> For every map created, the kernel will ensure
> spin_lock_off has correct value.
>
> Currently, bpf_map->spin_lock_off is not copied
> from the inner map to the map_in_map inner_map_meta
> during a map_in_map type map creation, so
> inner_map_meta->spin_lock_off = 0.
> This will give verifier wrong information that
> inner_map has bpf_spin_lock and the bpf_spin_lock
> is defined at offset 0. An access to offset 0
> of a value pointer will trigger the following error:
> bpf_spin_lock cannot be accessed directly by load/store
>
> This patch fixed the issue by copy inner map's spin_lock_off
> value to inner_map_meta->spin_lock_off.
>
> Fixes: d83525ca62cf ("bpf: introduce bpf_spin_lock")
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> kernel/bpf/map_in_map.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
> index 583346a0ab29..3dff41403583 100644
> --- a/kernel/bpf/map_in_map.c
> +++ b/kernel/bpf/map_in_map.c
> @@ -58,6 +58,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
> inner_map_meta->value_size = inner_map->value_size;
> inner_map_meta->map_flags = inner_map->map_flags;
> inner_map_meta->max_entries = inner_map->max_entries;
> + inner_map_meta->spin_lock_off = inner_map->spin_lock_off;
Looks like spinlock inside inner map is not supported: there is
specific check few lines above returning -ENOSUPP for such case. In
that case, maybe assign -1 here to make this explicit?
Though I guess that also brings up the question: is there any harm in
supporting spin lock for inner map and why it was disabled in the
first place?
>
> /* Misc members not needed in bpf_map_meta_equal() check. */
> inner_map_meta->ops = inner_map->ops;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog
From: Andrii Nakryiko @ 2019-02-27 23:36 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
In-Reply-To: <20190227212257.3856619-1-yhs@fb.com>
On Wed, Feb 27, 2019 at 1:24 PM Yonghong Song <yhs@fb.com> wrote:
>
> The bpf_map_lookup_elem is added in the bpf program.
> Without previous patch, the test change will trigger the
> following error:
> $ ./test_maps
> ...
> ; value_p = bpf_map_lookup_elem(map, &key);
> 20: (bf) r1 = r7
> 21: (bf) r2 = r8
> 22: (85) call bpf_map_lookup_elem#1
> ; if (!value_p || *value_p != 123)
> 23: (15) if r0 == 0x0 goto pc+16
> R0=map_value(id=2,off=0,ks=4,vs=4,imm=0) R6=inv1 R7=map_ptr(id=0,off=0,ks=4,vs=4,imm=0)
> R8=fp-8,call_-1 R10=fp0,call_-1 fp-8=mmmmmmmm
> ; if (!value_p || *value_p != 123)
> 24: (61) r1 = *(u32 *)(r0 +0)
> R0=map_value(id=2,off=0,ks=4,vs=4,imm=0) R6=inv1 R7=map_ptr(id=0,off=0,ks=4,vs=4,imm=0)
> R8=fp-8,call_-1 R10=fp0,call_-1 fp-8=mmmmmmmm
> bpf_spin_lock cannot be accessed directly by load/store
>
> With the kernel fix in the previous commit, the error goes away.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> tools/testing/selftests/bpf/progs/test_map_in_map.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map.c b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> index ce923e67e08e..2985f262846e 100644
> --- a/tools/testing/selftests/bpf/progs/test_map_in_map.c
> +++ b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> @@ -27,6 +27,7 @@ SEC("xdp_mimtest")
> int xdp_mimtest0(struct xdp_md *ctx)
> {
> int value = 123;
> + int *value_p;
> int key = 0;
> void *map;
>
> @@ -35,6 +36,9 @@ int xdp_mimtest0(struct xdp_md *ctx)
> return XDP_DROP;
>
> bpf_map_update_elem(map, &key, &value, 0);
> + value_p = bpf_map_lookup_elem(map, &key);
> + if (!value_p || *value_p != 123)
> + return XDP_DROP;
>
> map = bpf_map_lookup_elem(&mim_hash, &key);
> if (!map)
> --
> 2.17.1
>
LGTM.
Acked-by: Andrii Nakryiko <andriin@fb.com>
^ permalink raw reply
* Re: [PATCH net] net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode
From: Vivien Didelot @ 2019-02-27 23:40 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, Florian Fainelli, David Miller,
netdev@vger.kernel.org
In-Reply-To: <864ac062-51cb-ec9e-388b-b74a62eb6e6a@gmail.com>
Hi Heiner,
On Wed, 27 Feb 2019 20:55:22 +0100, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> When debugging another issue I faced an interrupt storm in this
> driver (88E6390, port 9 in SGMII mode), consisting of alternating
> link-up / link-down interrupts. Analysis showed that the driver
> wanted to set a cmode that was set already. But so far
> mv88e6390x_port_set_cmode() doesn't check this and powers down
> SERDES, what causes the link to break, and eventually results in
> the described interrupt storm.
>
> Fix this by checking whether the cmode actually changes. We want
> that the very first call to mv88e6390x_port_set_cmode() always
> configures the registers, therefore initialize port.cmode with
> a value that is different from any supported cmode value.
>
> Fixes: 364e9d7776a3 ("net: dsa: mv88e6xxx: Power on/off SERDES on cmode change")
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
> drivers/net/dsa/mv88e6xxx/port.c | 4 ++++
> drivers/net/dsa/mv88e6xxx/port.h | 1 +
> 3 files changed, 9 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 32e7af5ca..d4edb61e8 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -4568,6 +4568,7 @@ static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip)
> static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
> {
> struct mv88e6xxx_chip *chip;
> + int i;
>
> chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> if (!chip)
> @@ -4578,6 +4579,9 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
> mutex_init(&chip->reg_lock);
> INIT_LIST_HEAD(&chip->mdios);
>
> + for (i = 0; i < ARRAY_SIZE(chip->ports); i++)
mv88e6xxx_num_ports(chip)
> + chip->ports[i].cmode = MV88E6XXX_PORT_STS_CMODE_INVALID;
> +
> return chip;
> }
>
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index ebd26b6a9..70b7a1463 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -398,6 +398,10 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
> cmode = 0;
> }
>
> + /* cmode doesn't change, nothing to do for us */
> + if (cmode == chip->ports[port].cmode)
> + return 0;
> +
> lane = mv88e6390x_serdes_get_lane(chip, port);
> if (lane < 0)
> return lane;
> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
> index e583641de..4aadf321e 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.h
> +++ b/drivers/net/dsa/mv88e6xxx/port.h
> @@ -52,6 +52,7 @@
> #define MV88E6185_PORT_STS_CMODE_1000BASE_X 0x0005
> #define MV88E6185_PORT_STS_CMODE_PHY 0x0006
> #define MV88E6185_PORT_STS_CMODE_DISABLED 0x0007
> +#define MV88E6XXX_PORT_STS_CMODE_INVALID 0xff
Is this 0xff a mask value from the Port Status register? If so please
the 0x1234 format like above, to make this mask value obvious.
>
> /* Offset 0x01: MAC (or PCS or Physical) Control Register */
> #define MV88E6XXX_PORT_MAC_CTL 0x01
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define for map iteration
From: Andrii Nakryiko @ 2019-02-27 23:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-4-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> For historical reasons the helper to loop over maps in an object
> is called bpf_map__for_each while it really should be called
> bpf_object__for_each_map. Rename and add a correctly named
> define for backward compatibility.
Seems like there are at least 3 more functions that are not named correctly:
- __bpf_map__iter (__bpf_object__iter_map?)
- bpf_map__next (=> bpf_object__next_map?)
- bpf_map__prev (=> bpf_object__prev_map?)
Let's rename them as well?
>
> Switch all in-tree users to the correct name (Quentin).
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> tools/bpf/bpftool/prog.c | 4 ++--
> tools/lib/bpf/libbpf.c | 8 ++++----
> tools/lib/bpf/libbpf.h | 3 ++-
> tools/perf/util/bpf-loader.c | 4 ++--
> tools/testing/selftests/bpf/test_libbpf_open.c | 2 +-
> 5 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 0c35dd543d49..8ef80d65a474 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1053,7 +1053,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> j = 0;
> while (j < old_map_fds && map_replace[j].name) {
> i = 0;
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> if (!strcmp(bpf_map__name(map), map_replace[j].name)) {
> map_replace[j].idx = i;
> break;
> @@ -1074,7 +1074,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> /* Set ifindex and name reuse */
> j = 0;
> idx = 0;
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> if (!bpf_map__is_offload_neutral(map))
> bpf_map__set_ifindex(map, ifindex);
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b38dcbe7460a..f5eb60379c8d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2100,7 +2100,7 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
> if (err)
> return err;
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> char buf[PATH_MAX];
> int len;
>
> @@ -2147,7 +2147,7 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
> if (!obj)
> return -ENOENT;
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> char buf[PATH_MAX];
> int len;
>
> @@ -2835,7 +2835,7 @@ bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
> {
> struct bpf_map *pos;
>
> - bpf_map__for_each(pos, obj) {
> + bpf_object__for_each_map(pos, obj) {
> if (pos->name && !strcmp(pos->name, name))
> return pos;
> }
> @@ -2928,7 +2928,7 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> first_prog = prog;
> }
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> if (!bpf_map__is_offload_neutral(map))
> map->map_ifindex = attr->ifindex;
> }
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 6c0168f8bba5..b4652aa1a58a 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -278,10 +278,11 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset);
>
> LIBBPF_API struct bpf_map *
> bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
> -#define bpf_map__for_each(pos, obj) \
> +#define bpf_object__for_each_map(pos, obj) \
> for ((pos) = bpf_map__next(NULL, (obj)); \
> (pos) != NULL; \
> (pos) = bpf_map__next((pos), (obj)))
> +#define bpf_map__for_each bpf_object__for_each_map
Should we get rid of this as well, instead of accumulating cruft?
>
> LIBBPF_API struct bpf_map *
> bpf_map__prev(struct bpf_map *map, struct bpf_object *obj);
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 037d8ff6a634..31b7e5a1453b 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -1489,7 +1489,7 @@ apply_obj_config_object(struct bpf_object *obj)
> struct bpf_map *map;
> int err;
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> err = apply_obj_config_map(map);
> if (err)
> return err;
> @@ -1513,7 +1513,7 @@ int bpf__apply_obj_config(void)
>
> #define bpf__for_each_map(pos, obj, objtmp) \
> bpf_object__for_each_safe(obj, objtmp) \
> - bpf_map__for_each(pos, obj)
> + bpf_object__for_each_map(pos, obj)
>
> #define bpf__for_each_map_named(pos, obj, objtmp, name) \
> bpf__for_each_map(pos, obj, objtmp) \
> diff --git a/tools/testing/selftests/bpf/test_libbpf_open.c b/tools/testing/selftests/bpf/test_libbpf_open.c
> index 1909ecf4d999..65cbd30704b5 100644
> --- a/tools/testing/selftests/bpf/test_libbpf_open.c
> +++ b/tools/testing/selftests/bpf/test_libbpf_open.c
> @@ -67,7 +67,7 @@ int test_walk_maps(struct bpf_object *obj, bool verbose)
> struct bpf_map *map;
> int cnt = 0;
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> cnt++;
> if (verbose)
> printf("Map (count:%d) name: %s\n", cnt,
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH net] net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode
From: Heiner Kallweit @ 2019-02-27 23:48 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20190227184040.GB23759@t480s.localdomain>
On 28.02.2019 00:40, Vivien Didelot wrote:
> Hi Heiner,
>
> On Wed, 27 Feb 2019 20:55:22 +0100, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> When debugging another issue I faced an interrupt storm in this
>> driver (88E6390, port 9 in SGMII mode), consisting of alternating
>> link-up / link-down interrupts. Analysis showed that the driver
>> wanted to set a cmode that was set already. But so far
>> mv88e6390x_port_set_cmode() doesn't check this and powers down
>> SERDES, what causes the link to break, and eventually results in
>> the described interrupt storm.
>>
>> Fix this by checking whether the cmode actually changes. We want
>> that the very first call to mv88e6390x_port_set_cmode() always
>> configures the registers, therefore initialize port.cmode with
>> a value that is different from any supported cmode value.
>>
>> Fixes: 364e9d7776a3 ("net: dsa: mv88e6xxx: Power on/off SERDES on cmode change")
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
>> drivers/net/dsa/mv88e6xxx/port.c | 4 ++++
>> drivers/net/dsa/mv88e6xxx/port.h | 1 +
>> 3 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 32e7af5ca..d4edb61e8 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -4568,6 +4568,7 @@ static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip)
>> static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
>> {
>> struct mv88e6xxx_chip *chip;
>> + int i;
>>
>> chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
>> if (!chip)
>> @@ -4578,6 +4579,9 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
>> mutex_init(&chip->reg_lock);
>> INIT_LIST_HEAD(&chip->mdios);
>>
>> + for (i = 0; i < ARRAY_SIZE(chip->ports); i++)
>
> mv88e6xxx_num_ports(chip)
>
OK
>> + chip->ports[i].cmode = MV88E6XXX_PORT_STS_CMODE_INVALID;
>> +
>> return chip;
>> }
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
>> index ebd26b6a9..70b7a1463 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.c
>> +++ b/drivers/net/dsa/mv88e6xxx/port.c
>> @@ -398,6 +398,10 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
>> cmode = 0;
>> }
>>
>> + /* cmode doesn't change, nothing to do for us */
>> + if (cmode == chip->ports[port].cmode)
>> + return 0;
>> +
>> lane = mv88e6390x_serdes_get_lane(chip, port);
>> if (lane < 0)
>> return lane;
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
>> index e583641de..4aadf321e 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.h
>> +++ b/drivers/net/dsa/mv88e6xxx/port.h
>> @@ -52,6 +52,7 @@
>> #define MV88E6185_PORT_STS_CMODE_1000BASE_X 0x0005
>> #define MV88E6185_PORT_STS_CMODE_PHY 0x0006
>> #define MV88E6185_PORT_STS_CMODE_DISABLED 0x0007
>> +#define MV88E6XXX_PORT_STS_CMODE_INVALID 0xff
>
> Is this 0xff a mask value from the Port Status register? If so please
> the 0x1234 format like above, to make this mask value obvious.
>
No, it's not a mask. I couldn't use 0 for "invalid" because 0 is used
otherwise, so I went with 0xff. All these STS_CMODE values are also
stored in mv88e6xxx_port.cmode that has type u8. Therefore I decided
to write it as an 8 bit value.
>>
>> /* Offset 0x01: MAC (or PCS or Physical) Control Register */
>> #define MV88E6XXX_PORT_MAC_CTL 0x01
>
>
> Thanks,
>
> Vivien
>
Heiner
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Michael S. Tsirkin @ 2019-02-27 23:50 UTC (permalink / raw)
To: si-wei liu
Cc: Samudrala, Sridhar, Siwei Liu, Jiri Pirko, Stephen Hemminger,
David Miller, Netdev, virtualization, virtio-dev,
Brandeburg, Jesse, Alexander Duyck, Jakub Kicinski, Jason Wang,
liran.alon
In-Reply-To: <c72ce9eb-254c-cc3e-1969-f7f108506d5e@oracle.com>
On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
>
>
> On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
> > On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
> > >
> > > On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
> > > > On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
> > > > > On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
> > > > > > On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
> > > > > > > On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> > > > > > > > On 2/21/2019 7:33 PM, si-wei liu wrote:
> > > > > > > > > On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> > > > > > > > > > On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> > > > > > > > > > > Sorry for replying to this ancient thread. There was some remaining
> > > > > > > > > > > issue that I don't think the initial net_failover patch got addressed
> > > > > > > > > > > cleanly, see:
> > > > > > > > > > >
> > > > > > > > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> > > > > > > > > > >
> > > > > > > > > > > The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> > > > > > > > > > > not specifically writtten for such kernel automatic enslavement.
> > > > > > > > > > > Specifically, if it is a bond or team, the slave would typically get
> > > > > > > > > > > renamed *before* virtual device gets created, that's what udev can
> > > > > > > > > > > control (without getting netdev opened early by the other part of
> > > > > > > > > > > kernel) and other userspace components for e.g. initramfs,
> > > > > > > > > > > init-scripts can coordinate well in between. The in-kernel
> > > > > > > > > > > auto-enslavement of net_failover breaks this userspace convention,
> > > > > > > > > > > which don't provides a solution if user care about consistent naming
> > > > > > > > > > > on the slave netdevs specifically.
> > > > > > > > > > >
> > > > > > > > > > > Previously this issue had been specifically called out when IFF_HIDDEN
> > > > > > > > > > > and the 1-netdev was proposed, but no one gives out a solution to this
> > > > > > > > > > > problem ever since. Please share your mind how to proceed and solve
> > > > > > > > > > > this userspace issue if netdev does not welcome a 1-netdev model.
> > > > > > > > > > Above says:
> > > > > > > > > >
> > > > > > > > > > there's no motivation in the systemd/udevd community at
> > > > > > > > > > this point to refactor the rename logic and make it work well with
> > > > > > > > > > 3-netdev.
> > > > > > > > > >
> > > > > > > > > > What would the fix be? Skip slave devices?
> > > > > > > > > >
> > > > > > > > > There's nothing user can get if just skipping slave devices - the
> > > > > > > > > name is still unchanged and unpredictable e.g. eth0, or eth1 the
> > > > > > > > > next reboot, while the rest may conform to the naming scheme (ens3
> > > > > > > > > and such). There's no way one can fix this in userspace alone - when
> > > > > > > > > the failover is created the enslaved netdev was opened by the kernel
> > > > > > > > > earlier than the userspace is made aware of, and there's no
> > > > > > > > > negotiation protocol for kernel to know when userspace has done
> > > > > > > > > initial renaming of the interface. I would expect netdev list should
> > > > > > > > > at least provide the direction in general for how this can be
> > > > > > > > > solved...
> > > > > > I was just wondering what did you mean when you said
> > > > > > "refactor the rename logic and make it work well with 3-netdev" -
> > > > > > was there a proposal udev rejected?
> > > > > No. I never believed this particular issue can be fixed in userspace alone.
> > > > > Previously someone had said it could be, but I never see any work or
> > > > > relevant discussion ever happened in various userspace communities (for e.g.
> > > > > dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
> > > > > of the issue derives from the kernel, it makes more sense to start from
> > > > > netdev, work out and decide on a solution: see what can be done in the
> > > > > kernel in order to fix it, then after that engage userspace community for
> > > > > the feasibility...
> > > > >
> > > > > > Anyway, can we write a time diagram for what happens in which order that
> > > > > > leads to failure? That would help look for triggers that we can tie
> > > > > > into, or add new ones.
> > > > > >
> > > > > See attached diagram.
> > > > >
> > > > > >
> > > > > >
> > > > > > > > Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> > > > > > > > to only work with the master failover device.
> > > > > > > Where does this expectation come from?
> > > > > > >
> > > > > > > Admin users may have ethtool or tc configurations that need to deal with
> > > > > > > predictable interface name. Third-party app which was built upon specifying
> > > > > > > certain interface name can't be modified to chase dynamic names.
> > > > > > >
> > > > > > > Specifically, we have pre-canned image that uses ethtool to fine tune VF
> > > > > > > offload settings post boot for specific workload. Those images won't work
> > > > > > > well if the name is constantly changing just after couple rounds of live
> > > > > > > migration.
> > > > > > It should be possible to specify the ethtool configuration on the
> > > > > > master and have it automatically propagated to the slave.
> > > > > >
> > > > > > BTW this is something we should look at IMHO.
> > > > > I was elaborating a few examples that the expectation and assumption that
> > > > > user/admin scripts only deal with master failover device is incorrect. It
> > > > > had never been taken good care of, although I did try to emphasize it from
> > > > > the very beginning.
> > > > >
> > > > > Basically what you said about propagating the ethtool configuration down to
> > > > > the slave is the key pursuance of 1-netdev model. However, what I am seeking
> > > > > now is any alternative that can also fix the specific udev rename problem,
> > > > > before concluding that 1-netdev is the only solution. Generally a 1-netdev
> > > > > scheme would take time to implement, while I'm trying to find a way out to
> > > > > fix this particular naming problem under 3-netdev.
> > > > >
> > > > > > > > Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> > > > > > > > about moving them to a hidden network namespace so that they are not visible from the default namespace.
> > > > > > > > I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> > > > > > > > kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> > > > > > > Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> > > > > > > model as much transparent to a real NIC as possible, while a hidden netns is
> > > > > > > just the vehicle). However, I recall there was resistance around this
> > > > > > > discussion that even the concept of hiding itself is a taboo for Linux
> > > > > > > netdev. I would like to summon potential alternatives before concluding
> > > > > > > 1-netdev is the only solution too soon.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > -Siwei
> > > > > > Your scripts would not work at all then, right?
> > > > > At this point we don't claim images with such usage as SR-IOV live
> > > > > migrate-able. We would flag it as live migrate-able until this ethtool
> > > > > config issue is fully addressed and a transparent live migration solution
> > > > > emerges in upstream eventually.
> > > > >
> > > > >
> > > > > Thanks,
> > > > > -Siwei
> > > > > > > > > -Siwei
> > > > > > > > >
> > > > > > > > >
> > > > > > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > >
> > > > > net_failover(kernel) | network.service (user) | systemd-udevd (user)
> > > > > --------------------------------------------------+------------------------------+--------------------------------------------
> > > > > (standby virtio-net and net_failover | |
> > > > > devices created and initialized, | |
> > > > > i.e. virtnet_probe()-> | |
> > > > > net_failover_create() | |
> > > > > was done.) | |
> > > > > | |
> > > > > | runs `ifup ens3' -> |
> > > > > | ip link set dev ens3 up |
> > > > > net_failover_open() | |
> > > > > dev_open(virtnet_dev) | |
> > > > > virtnet_open(virtnet_dev) | |
> > > > > netif_carrier_on(failover_dev) | |
> > > > > ... | |
> > > > > | |
> > > > > (VF hot plugged in) | |
> > > > > ixgbevf_probe() | |
> > > > > register_netdev(ixgbevf_netdev) | |
> > > > > netdev_register_kobject(ixgbevf_netdev) | |
> > > > > kobject_add(ixgbevf_dev) | |
> > > > > device_add(ixgbevf_dev) | |
> > > > > kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
> > > > > netlink_broadcast() | |
> > > > > ... | |
> > > > > call_netdevice_notifiers(NETDEV_REGISTER) | |
> > > > > failover_event(..., NETDEV_REGISTER, ...) | |
> > > > > failover_slave_register(ixgbevf_netdev) | |
> > > > > net_failover_slave_register(ixgbevf_netdev) | |
> > > > > dev_open(ixgbevf_netdev) | |
> > > > > | |
> > > > > | |
> > > > > | | received ADD uevent from netlink fd
> > > > > | | ...
> > > > > | | udev-builtin-net_id.c:dev_pci_slot()
> > > > > | | (decided to renamed 'eth0' )
> > > > > | | ip link set dev eth0 name ens4
> > > > > (dev_change_name() returns -EBUSY as | |
> > > > > ixgbevf_netdev->flags has IFF_UP) | |
> > > > > | |
> > > > >
> > > > Given renaming slaves does not work anyway:
> > > I was actually thinking what if we relieve the rename restriction just for
> > > the failover slave? What the impact would be? I think users don't care about
> > > slave being renamed when it's in use, especially the initial rename.
> > > Thoughts?
> > >
> > > > would it work if we just
> > > > hard-coded slave names instead?
> > > >
> > > > E.g.
> > > > 1. fail slave renames
> > > > 2. rename of failover to XX automatically renames standby to XXnsby
> > > > and primary to XXnpry
> > > That wouldn't help. The time when the failover master gets renamed, the VF
> > > may not be present.
> > In this scheme if VF is not there it will be renamed immediately after registration.
> Who will be responsible to rename the slave, the kernel?
That's the idea.
> Note the master's
> name may or may not come from the userspace. If it comes from the userspace,
> should the userspace daemon change their expectation not to name/rename
> _any_ slaves (today there's no distinction)?
Yes the idea would be to fail renaming slaves.
> How do users know which name to
> trust, depending on which wins the race more often? Say if kernel wants a
> ens3npry name while userspace wants it named as ens4.
>
> -Siwei
With this approach kernel will deny attempts by userspace to rename
slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
will rename both slaves.
It seems pretty solid to me, the only issue is that in theory userspace
can use a name like XXXnsby for something else. But this seems unlikely.
> >
> > > I don't like the idea to delay exposing failover master
> > > until VF is hot plugged in (probably subject to various failures) later.
> > >
> > > Thanks,
> > > -Siwei
> >
> > I agree, this was not what I meant.
> >
> > > >
^ permalink raw reply
* Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Heiner Kallweit @ 2019-02-27 23:51 UTC (permalink / raw)
To: Andrew Lunn, Russell King - ARM Linux admin
Cc: Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190227231940.GN3809@lunn.ch>
On 28.02.2019 00:19, Andrew Lunn wrote:
>> >From what you've described, it sounds like what you actually have is:
>>
>> MAC <---> Serdes PHY <---> PHY
>>
>> The Serdes PHY receives the SGMII in-band negotiation from the external
>> PHY, but there is no propagation of the status from the serdes PHY to
>> the MAC.
>
> Yes, that is a good description. So far, we have not yet got the MAC
> to read the speed and duplex from the SERDES to configure itself. In
> theory it should be able to, it is all in the same device.
>
> It might be that once the SERDES interrupts saying it has link we need
> to program the MAC with the result of the in-band signalling. in-band
> then seems a bit pointless. Or we are missing some configuration
> somewhere to tell the MAC to use the in-band signalling result from
> the SERDES.
>
I'll play a little with it and see whether I get it working.
As a fallback we have the option to go with out-of-band.
> Andrew
>
>
Heiner
^ permalink raw reply
* Re: [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define for map iteration
From: Jakub Kicinski @ 2019-02-27 23:57 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <CAEf4BzZR+7R9TjXSMHiYzbHCvuwL+Z_LBD=Pk+rRW+X+pgdg4w@mail.gmail.com>
On Wed, 27 Feb 2019 15:47:56 -0800, Andrii Nakryiko wrote:
> On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> >
> > For historical reasons the helper to loop over maps in an object
> > is called bpf_map__for_each while it really should be called
> > bpf_object__for_each_map. Rename and add a correctly named
> > define for backward compatibility.
>
> Seems like there are at least 3 more functions that are not named correctly:
> - __bpf_map__iter (__bpf_object__iter_map?)
> - bpf_map__next (=> bpf_object__next_map?)
> - bpf_map__prev (=> bpf_object__prev_map?)
>
> Let's rename them as well?
At least those are consistently named between programs and maps.
I'm happy to do the rename if we don't need backward compat, seems
a little much to add aliases?
> > Switch all in-tree users to the correct name (Quentin).
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 6c0168f8bba5..b4652aa1a58a 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -278,10 +278,11 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset);
> >
> > LIBBPF_API struct bpf_map *
> > bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
> > -#define bpf_map__for_each(pos, obj) \
> > +#define bpf_object__for_each_map(pos, obj) \
> > for ((pos) = bpf_map__next(NULL, (obj)); \
> > (pos) != NULL; \
> > (pos) = bpf_map__next((pos), (obj)))
> > +#define bpf_map__for_each bpf_object__for_each_map
>
> Should we get rid of this as well, instead of accumulating cruft?
Well, we did some gymnastics in the past to maintain backward compat,
I thought we do need it..?
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Liran Alon @ 2019-02-28 0:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: si-wei liu, Samudrala, Sridhar, Siwei Liu, Jiri Pirko,
Stephen Hemminger, David Miller, Netdev, virtualization,
virtio-dev, Brandeburg, Jesse, Alexander Duyck, Jakub Kicinski,
Jason Wang
In-Reply-To: <20190227184601-mutt-send-email-mst@kernel.org>
> On 28 Feb 2019, at 1:50, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
>>
>>
>> On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
>>> On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
>>>>
>>>> On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
>>>>> On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
>>>>>> On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
>>>>>>>> On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
>>>>>>>>> On 2/21/2019 7:33 PM, si-wei liu wrote:
>>>>>>>>>> On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
>>>>>>>>>>> On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
>>>>>>>>>>>> Sorry for replying to this ancient thread. There was some remaining
>>>>>>>>>>>> issue that I don't think the initial net_failover patch got addressed
>>>>>>>>>>>> cleanly, see:
>>>>>>>>>>>>
>>>>>>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.launchpad.net_ubuntu_-2Bsource_linux_-2Bbug_1815268&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=aL-QfUoSYx8r0XCOBkcDtF8f-cYxrJI3skYLFTb8XJE&s=yk6Nqv3a6_JMzyrXKY67h00FyNrDJyQ-PYMFffDSTXM&e=
>>>>>>>>>>>>
>>>>>>>>>>>> The renaming of 'eth0' to 'ens4' fails because the udev userspace was
>>>>>>>>>>>> not specifically writtten for such kernel automatic enslavement.
>>>>>>>>>>>> Specifically, if it is a bond or team, the slave would typically get
>>>>>>>>>>>> renamed *before* virtual device gets created, that's what udev can
>>>>>>>>>>>> control (without getting netdev opened early by the other part of
>>>>>>>>>>>> kernel) and other userspace components for e.g. initramfs,
>>>>>>>>>>>> init-scripts can coordinate well in between. The in-kernel
>>>>>>>>>>>> auto-enslavement of net_failover breaks this userspace convention,
>>>>>>>>>>>> which don't provides a solution if user care about consistent naming
>>>>>>>>>>>> on the slave netdevs specifically.
>>>>>>>>>>>>
>>>>>>>>>>>> Previously this issue had been specifically called out when IFF_HIDDEN
>>>>>>>>>>>> and the 1-netdev was proposed, but no one gives out a solution to this
>>>>>>>>>>>> problem ever since. Please share your mind how to proceed and solve
>>>>>>>>>>>> this userspace issue if netdev does not welcome a 1-netdev model.
>>>>>>>>>>> Above says:
>>>>>>>>>>>
>>>>>>>>>>> there's no motivation in the systemd/udevd community at
>>>>>>>>>>> this point to refactor the rename logic and make it work well with
>>>>>>>>>>> 3-netdev.
>>>>>>>>>>>
>>>>>>>>>>> What would the fix be? Skip slave devices?
>>>>>>>>>>>
>>>>>>>>>> There's nothing user can get if just skipping slave devices - the
>>>>>>>>>> name is still unchanged and unpredictable e.g. eth0, or eth1 the
>>>>>>>>>> next reboot, while the rest may conform to the naming scheme (ens3
>>>>>>>>>> and such). There's no way one can fix this in userspace alone - when
>>>>>>>>>> the failover is created the enslaved netdev was opened by the kernel
>>>>>>>>>> earlier than the userspace is made aware of, and there's no
>>>>>>>>>> negotiation protocol for kernel to know when userspace has done
>>>>>>>>>> initial renaming of the interface. I would expect netdev list should
>>>>>>>>>> at least provide the direction in general for how this can be
>>>>>>>>>> solved...
>>>>>>> I was just wondering what did you mean when you said
>>>>>>> "refactor the rename logic and make it work well with 3-netdev" -
>>>>>>> was there a proposal udev rejected?
>>>>>> No. I never believed this particular issue can be fixed in userspace alone.
>>>>>> Previously someone had said it could be, but I never see any work or
>>>>>> relevant discussion ever happened in various userspace communities (for e.g.
>>>>>> dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
>>>>>> of the issue derives from the kernel, it makes more sense to start from
>>>>>> netdev, work out and decide on a solution: see what can be done in the
>>>>>> kernel in order to fix it, then after that engage userspace community for
>>>>>> the feasibility...
>>>>>>
>>>>>>> Anyway, can we write a time diagram for what happens in which order that
>>>>>>> leads to failure? That would help look for triggers that we can tie
>>>>>>> into, or add new ones.
>>>>>>>
>>>>>> See attached diagram.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>> Is there an issue if slave device names are not predictable? The user/admin scripts are expected
>>>>>>>>> to only work with the master failover device.
>>>>>>>> Where does this expectation come from?
>>>>>>>>
>>>>>>>> Admin users may have ethtool or tc configurations that need to deal with
>>>>>>>> predictable interface name. Third-party app which was built upon specifying
>>>>>>>> certain interface name can't be modified to chase dynamic names.
>>>>>>>>
>>>>>>>> Specifically, we have pre-canned image that uses ethtool to fine tune VF
>>>>>>>> offload settings post boot for specific workload. Those images won't work
>>>>>>>> well if the name is constantly changing just after couple rounds of live
>>>>>>>> migration.
>>>>>>> It should be possible to specify the ethtool configuration on the
>>>>>>> master and have it automatically propagated to the slave.
>>>>>>>
>>>>>>> BTW this is something we should look at IMHO.
>>>>>> I was elaborating a few examples that the expectation and assumption that
>>>>>> user/admin scripts only deal with master failover device is incorrect. It
>>>>>> had never been taken good care of, although I did try to emphasize it from
>>>>>> the very beginning.
>>>>>>
>>>>>> Basically what you said about propagating the ethtool configuration down to
>>>>>> the slave is the key pursuance of 1-netdev model. However, what I am seeking
>>>>>> now is any alternative that can also fix the specific udev rename problem,
>>>>>> before concluding that 1-netdev is the only solution. Generally a 1-netdev
>>>>>> scheme would take time to implement, while I'm trying to find a way out to
>>>>>> fix this particular naming problem under 3-netdev.
>>>>>>
>>>>>>>>> Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
>>>>>>>>> about moving them to a hidden network namespace so that they are not visible from the default namespace.
>>>>>>>>> I looked into this sometime back, but did not find the right kernel api to create a network namespace within
>>>>>>>>> kernel. If so, we could use this mechanism to simulate a 1-netdev model.
>>>>>>>> Yes, that's one possible implementation (IMHO the key is to make 1-netdev
>>>>>>>> model as much transparent to a real NIC as possible, while a hidden netns is
>>>>>>>> just the vehicle). However, I recall there was resistance around this
>>>>>>>> discussion that even the concept of hiding itself is a taboo for Linux
>>>>>>>> netdev. I would like to summon potential alternatives before concluding
>>>>>>>> 1-netdev is the only solution too soon.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> -Siwei
>>>>>>> Your scripts would not work at all then, right?
>>>>>> At this point we don't claim images with such usage as SR-IOV live
>>>>>> migrate-able. We would flag it as live migrate-able until this ethtool
>>>>>> config issue is fully addressed and a transparent live migration solution
>>>>>> emerges in upstream eventually.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> -Siwei
>>>>>>>>>> -Siwei
>>>>>>>>>>
>>>>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>>>>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>>>>>>
>>>>>> net_failover(kernel) | network.service (user) | systemd-udevd (user)
>>>>>> --------------------------------------------------+------------------------------+--------------------------------------------
>>>>>> (standby virtio-net and net_failover | |
>>>>>> devices created and initialized, | |
>>>>>> i.e. virtnet_probe()-> | |
>>>>>> net_failover_create() | |
>>>>>> was done.) | |
>>>>>> | |
>>>>>> | runs `ifup ens3' -> |
>>>>>> | ip link set dev ens3 up |
>>>>>> net_failover_open() | |
>>>>>> dev_open(virtnet_dev) | |
>>>>>> virtnet_open(virtnet_dev) | |
>>>>>> netif_carrier_on(failover_dev) | |
>>>>>> ... | |
>>>>>> | |
>>>>>> (VF hot plugged in) | |
>>>>>> ixgbevf_probe() | |
>>>>>> register_netdev(ixgbevf_netdev) | |
>>>>>> netdev_register_kobject(ixgbevf_netdev) | |
>>>>>> kobject_add(ixgbevf_dev) | |
>>>>>> device_add(ixgbevf_dev) | |
>>>>>> kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
>>>>>> netlink_broadcast() | |
>>>>>> ... | |
>>>>>> call_netdevice_notifiers(NETDEV_REGISTER) | |
>>>>>> failover_event(..., NETDEV_REGISTER, ...) | |
>>>>>> failover_slave_register(ixgbevf_netdev) | |
>>>>>> net_failover_slave_register(ixgbevf_netdev) | |
>>>>>> dev_open(ixgbevf_netdev) | |
>>>>>> | |
>>>>>> | |
>>>>>> | | received ADD uevent from netlink fd
>>>>>> | | ...
>>>>>> | | udev-builtin-net_id.c:dev_pci_slot()
>>>>>> | | (decided to renamed 'eth0' )
>>>>>> | | ip link set dev eth0 name ens4
>>>>>> (dev_change_name() returns -EBUSY as | |
>>>>>> ixgbevf_netdev->flags has IFF_UP) | |
>>>>>> | |
>>>>>>
>>>>> Given renaming slaves does not work anyway:
>>>> I was actually thinking what if we relieve the rename restriction just for
>>>> the failover slave? What the impact would be? I think users don't care about
>>>> slave being renamed when it's in use, especially the initial rename.
>>>> Thoughts?
>>>>
>>>>> would it work if we just
>>>>> hard-coded slave names instead?
>>>>>
>>>>> E.g.
>>>>> 1. fail slave renames
>>>>> 2. rename of failover to XX automatically renames standby to XXnsby
>>>>> and primary to XXnpry
>>>> That wouldn't help. The time when the failover master gets renamed, the VF
>>>> may not be present.
>>> In this scheme if VF is not there it will be renamed immediately after registration.
>> Who will be responsible to rename the slave, the kernel?
>
> That's the idea.
>
>> Note the master's
>> name may or may not come from the userspace. If it comes from the userspace,
>> should the userspace daemon change their expectation not to name/rename
>> _any_ slaves (today there's no distinction)?
>
> Yes the idea would be to fail renaming slaves.
>
>> How do users know which name to
>> trust, depending on which wins the race more often? Say if kernel wants a
>> ens3npry name while userspace wants it named as ens4.
>>
>> -Siwei
>
> With this approach kernel will deny attempts by userspace to rename
> slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> will rename both slaves.
>
> It seems pretty solid to me, the only issue is that in theory userspace
> can use a name like XXXnsby for something else. But this seems unlikely.
I’m fond of this idea and I have similar opinion.
I think it simplifies the issue here.
I don’t see a real reason for customer to define udev rule to rename a net-failover slave to have different postfix.
-Liran
>
>
>>>
>>>> I don't like the idea to delay exposing failover master
>>>> until VF is hot plugged in (probably subject to various failures) later.
>>>>
>>>> Thanks,
>>>> -Siwei
>>>
>>> I agree, this was not what I meant.
>>>
>>>>>
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Stephen Hemminger @ 2019-02-28 0:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: si-wei liu, Samudrala, Sridhar, Siwei Liu, Jiri Pirko,
David Miller, Netdev, virtualization, virtio-dev,
Brandeburg, Jesse, Alexander Duyck, Jakub Kicinski, Jason Wang,
liran.alon
In-Reply-To: <20190227184601-mutt-send-email-mst@kernel.org>
On Wed, 27 Feb 2019 18:50:44 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
> >
> >
> > On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
> > > On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
> > > >
> > > > On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
> > > > > On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
> > > > > > On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
> > > > > > > > On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> > > > > > > > > On 2/21/2019 7:33 PM, si-wei liu wrote:
> > > > > > > > > > On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> > > > > > > > > > > On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> > > > > > > > > > > > Sorry for replying to this ancient thread. There was some remaining
> > > > > > > > > > > > issue that I don't think the initial net_failover patch got addressed
> > > > > > > > > > > > cleanly, see:
> > > > > > > > > > > >
> > > > > > > > > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> > > > > > > > > > > >
> > > > > > > > > > > > The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> > > > > > > > > > > > not specifically writtten for such kernel automatic enslavement.
> > > > > > > > > > > > Specifically, if it is a bond or team, the slave would typically get
> > > > > > > > > > > > renamed *before* virtual device gets created, that's what udev can
> > > > > > > > > > > > control (without getting netdev opened early by the other part of
> > > > > > > > > > > > kernel) and other userspace components for e.g. initramfs,
> > > > > > > > > > > > init-scripts can coordinate well in between. The in-kernel
> > > > > > > > > > > > auto-enslavement of net_failover breaks this userspace convention,
> > > > > > > > > > > > which don't provides a solution if user care about consistent naming
> > > > > > > > > > > > on the slave netdevs specifically.
> > > > > > > > > > > >
> > > > > > > > > > > > Previously this issue had been specifically called out when IFF_HIDDEN
> > > > > > > > > > > > and the 1-netdev was proposed, but no one gives out a solution to this
> > > > > > > > > > > > problem ever since. Please share your mind how to proceed and solve
> > > > > > > > > > > > this userspace issue if netdev does not welcome a 1-netdev model.
> > > > > > > > > > > Above says:
> > > > > > > > > > >
> > > > > > > > > > > there's no motivation in the systemd/udevd community at
> > > > > > > > > > > this point to refactor the rename logic and make it work well with
> > > > > > > > > > > 3-netdev.
> > > > > > > > > > >
> > > > > > > > > > > What would the fix be? Skip slave devices?
> > > > > > > > > > >
> > > > > > > > > > There's nothing user can get if just skipping slave devices - the
> > > > > > > > > > name is still unchanged and unpredictable e.g. eth0, or eth1 the
> > > > > > > > > > next reboot, while the rest may conform to the naming scheme (ens3
> > > > > > > > > > and such). There's no way one can fix this in userspace alone - when
> > > > > > > > > > the failover is created the enslaved netdev was opened by the kernel
> > > > > > > > > > earlier than the userspace is made aware of, and there's no
> > > > > > > > > > negotiation protocol for kernel to know when userspace has done
> > > > > > > > > > initial renaming of the interface. I would expect netdev list should
> > > > > > > > > > at least provide the direction in general for how this can be
> > > > > > > > > > solved...
> > > > > > > I was just wondering what did you mean when you said
> > > > > > > "refactor the rename logic and make it work well with 3-netdev" -
> > > > > > > was there a proposal udev rejected?
> > > > > > No. I never believed this particular issue can be fixed in userspace alone.
> > > > > > Previously someone had said it could be, but I never see any work or
> > > > > > relevant discussion ever happened in various userspace communities (for e.g.
> > > > > > dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
> > > > > > of the issue derives from the kernel, it makes more sense to start from
> > > > > > netdev, work out and decide on a solution: see what can be done in the
> > > > > > kernel in order to fix it, then after that engage userspace community for
> > > > > > the feasibility...
> > > > > >
> > > > > > > Anyway, can we write a time diagram for what happens in which order that
> > > > > > > leads to failure? That would help look for triggers that we can tie
> > > > > > > into, or add new ones.
> > > > > > >
> > > > > > See attached diagram.
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > > Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> > > > > > > > > to only work with the master failover device.
> > > > > > > > Where does this expectation come from?
> > > > > > > >
> > > > > > > > Admin users may have ethtool or tc configurations that need to deal with
> > > > > > > > predictable interface name. Third-party app which was built upon specifying
> > > > > > > > certain interface name can't be modified to chase dynamic names.
> > > > > > > >
> > > > > > > > Specifically, we have pre-canned image that uses ethtool to fine tune VF
> > > > > > > > offload settings post boot for specific workload. Those images won't work
> > > > > > > > well if the name is constantly changing just after couple rounds of live
> > > > > > > > migration.
> > > > > > > It should be possible to specify the ethtool configuration on the
> > > > > > > master and have it automatically propagated to the slave.
> > > > > > >
> > > > > > > BTW this is something we should look at IMHO.
> > > > > > I was elaborating a few examples that the expectation and assumption that
> > > > > > user/admin scripts only deal with master failover device is incorrect. It
> > > > > > had never been taken good care of, although I did try to emphasize it from
> > > > > > the very beginning.
> > > > > >
> > > > > > Basically what you said about propagating the ethtool configuration down to
> > > > > > the slave is the key pursuance of 1-netdev model. However, what I am seeking
> > > > > > now is any alternative that can also fix the specific udev rename problem,
> > > > > > before concluding that 1-netdev is the only solution. Generally a 1-netdev
> > > > > > scheme would take time to implement, while I'm trying to find a way out to
> > > > > > fix this particular naming problem under 3-netdev.
> > > > > >
> > > > > > > > > Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> > > > > > > > > about moving them to a hidden network namespace so that they are not visible from the default namespace.
> > > > > > > > > I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> > > > > > > > > kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> > > > > > > > Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> > > > > > > > model as much transparent to a real NIC as possible, while a hidden netns is
> > > > > > > > just the vehicle). However, I recall there was resistance around this
> > > > > > > > discussion that even the concept of hiding itself is a taboo for Linux
> > > > > > > > netdev. I would like to summon potential alternatives before concluding
> > > > > > > > 1-netdev is the only solution too soon.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > -Siwei
> > > > > > > Your scripts would not work at all then, right?
> > > > > > At this point we don't claim images with such usage as SR-IOV live
> > > > > > migrate-able. We would flag it as live migrate-able until this ethtool
> > > > > > config issue is fully addressed and a transparent live migration solution
> > > > > > emerges in upstream eventually.
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > -Siwei
> > > > > > > > > > -Siwei
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > ---------------------------------------------------------------------
> > > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > > >
> > > > > > net_failover(kernel) | network.service (user) | systemd-udevd (user)
> > > > > > --------------------------------------------------+------------------------------+--------------------------------------------
> > > > > > (standby virtio-net and net_failover | |
> > > > > > devices created and initialized, | |
> > > > > > i.e. virtnet_probe()-> | |
> > > > > > net_failover_create() | |
> > > > > > was done.) | |
> > > > > > | |
> > > > > > | runs `ifup ens3' -> |
> > > > > > | ip link set dev ens3 up |
> > > > > > net_failover_open() | |
> > > > > > dev_open(virtnet_dev) | |
> > > > > > virtnet_open(virtnet_dev) | |
> > > > > > netif_carrier_on(failover_dev) | |
> > > > > > ... | |
> > > > > > | |
> > > > > > (VF hot plugged in) | |
> > > > > > ixgbevf_probe() | |
> > > > > > register_netdev(ixgbevf_netdev) | |
> > > > > > netdev_register_kobject(ixgbevf_netdev) | |
> > > > > > kobject_add(ixgbevf_dev) | |
> > > > > > device_add(ixgbevf_dev) | |
> > > > > > kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
> > > > > > netlink_broadcast() | |
> > > > > > ... | |
> > > > > > call_netdevice_notifiers(NETDEV_REGISTER) | |
> > > > > > failover_event(..., NETDEV_REGISTER, ...) | |
> > > > > > failover_slave_register(ixgbevf_netdev) | |
> > > > > > net_failover_slave_register(ixgbevf_netdev) | |
> > > > > > dev_open(ixgbevf_netdev) | |
> > > > > > | |
> > > > > > | |
> > > > > > | | received ADD uevent from netlink fd
> > > > > > | | ...
> > > > > > | | udev-builtin-net_id.c:dev_pci_slot()
> > > > > > | | (decided to renamed 'eth0' )
> > > > > > | | ip link set dev eth0 name ens4
> > > > > > (dev_change_name() returns -EBUSY as | |
> > > > > > ixgbevf_netdev->flags has IFF_UP) | |
> > > > > > | |
> > > > > >
> > > > > Given renaming slaves does not work anyway:
> > > > I was actually thinking what if we relieve the rename restriction just for
> > > > the failover slave? What the impact would be? I think users don't care about
> > > > slave being renamed when it's in use, especially the initial rename.
> > > > Thoughts?
> > > >
> > > > > would it work if we just
> > > > > hard-coded slave names instead?
> > > > >
> > > > > E.g.
> > > > > 1. fail slave renames
> > > > > 2. rename of failover to XX automatically renames standby to XXnsby
> > > > > and primary to XXnpry
> > > > That wouldn't help. The time when the failover master gets renamed, the VF
> > > > may not be present.
> > > In this scheme if VF is not there it will be renamed immediately after registration.
> > Who will be responsible to rename the slave, the kernel?
>
> That's the idea.
>
> > Note the master's
> > name may or may not come from the userspace. If it comes from the userspace,
> > should the userspace daemon change their expectation not to name/rename
> > _any_ slaves (today there's no distinction)?
>
> Yes the idea would be to fail renaming slaves.
>
> > How do users know which name to
> > trust, depending on which wins the race more often? Say if kernel wants a
> > ens3npry name while userspace wants it named as ens4.
> >
> > -Siwei
>
> With this approach kernel will deny attempts by userspace to rename
> slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> will rename both slaves.
>
> It seems pretty solid to me, the only issue is that in theory userspace
> can use a name like XXXnsby for something else. But this seems unlikely.
Similar schemes (with kernel providing naming) were also previously rejected
upstream. It has been a consistent theme that the kernel should not be in
the renaming business. It will certainly break userspace.
^ permalink raw reply
* Re: [PATCH bpf-next 4/5] samples: bpf: use libbpf where easy
From: Andrii Nakryiko @ 2019-02-28 0:05 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-5-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> Some samples don't really need the magic of bpf_load,
> switch them to libbpf.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> samples/bpf/Makefile | 6 +++---
> samples/bpf/fds_example.c | 9 ++++++---
> samples/bpf/sockex1_user.c | 22 ++++++++++++----------
> samples/bpf/sockex2_user.c | 20 +++++++++++---------
> 4 files changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 4dd98100678e..0c62ac39c697 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -59,9 +59,9 @@ LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
> CGROUP_HELPERS := ../../tools/testing/selftests/bpf/cgroup_helpers.o
> TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
>
> -fds_example-objs := bpf_load.o fds_example.o
> -sockex1-objs := bpf_load.o sockex1_user.o
> -sockex2-objs := bpf_load.o sockex2_user.o
> +fds_example-objs := fds_example.o
> +sockex1-objs := sockex1_user.o
> +sockex2-objs := sockex2_user.o
> sockex3-objs := bpf_load.o sockex3_user.o
> tracex1-objs := bpf_load.o tracex1_user.o
> tracex2-objs := bpf_load.o tracex2_user.o
> diff --git a/samples/bpf/fds_example.c b/samples/bpf/fds_example.c
> index 9854854f05d1..36f1f18aae3c 100644
> --- a/samples/bpf/fds_example.c
> +++ b/samples/bpf/fds_example.c
> @@ -14,8 +14,8 @@
>
> #include <bpf/bpf.h>
>
> +#include "bpf/libbpf.h"
> #include "bpf_insn.h"
> -#include "bpf_load.h"
> #include "sock_example.h"
>
> #define BPF_F_PIN (1 << 0)
> @@ -57,10 +57,13 @@ static int bpf_prog_create(const char *object)
> BPF_EXIT_INSN(),
> };
> size_t insns_cnt = sizeof(insns) / sizeof(struct bpf_insn);
> + char bpf_log_buf[BPF_LOG_BUF_SIZE];
> + struct bpf_object *obj;
> + int prog_fd;
>
> if (object) {
> - assert(!load_bpf_file((char *)object));
> - return prog_fd[0];
> + assert(!bpf_prog_load(object, 0, &obj, &prog_fd));
Here and in few more places below: is it possible to specify correct
bpf_prog_type instead of 0?
> + return prog_fd;
> } else {
> return bpf_load_program(BPF_PROG_TYPE_SOCKET_FILTER,
> insns, insns_cnt, "GPL", 0,
> diff --git a/samples/bpf/sockex1_user.c b/samples/bpf/sockex1_user.c
> index be8ba5686924..5e7c4be3e645 100644
> --- a/samples/bpf/sockex1_user.c
> +++ b/samples/bpf/sockex1_user.c
> @@ -3,28 +3,30 @@
> #include <assert.h>
> #include <linux/bpf.h>
> #include <bpf/bpf.h>
> -#include "bpf_load.h"
> +#include "bpf/libbpf.h"
> #include "sock_example.h"
> #include <unistd.h>
> #include <arpa/inet.h>
>
> int main(int ac, char **argv)
> {
> + struct bpf_object *obj;
> + int map_fd, prog_fd;
> char filename[256];
> - FILE *f;
> int i, sock;
> + FILE *f;
>
> snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
>
> - if (load_bpf_file(filename)) {
> - printf("%s", bpf_log_buf);
> + if (bpf_prog_load(filename, 0, &obj, &prog_fd))
> return 1;
> - }
> +
> + map_fd = bpf_object__find_map_fd_by_name(obj, "my_map");
>
> sock = open_raw_sock("lo");
>
> - assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
> - sizeof(prog_fd[0])) == 0);
> + assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd,
> + sizeof(prog_fd)) == 0);
>
> f = popen("ping -4 -c5 localhost", "r");
> (void) f;
> @@ -34,13 +36,13 @@ int main(int ac, char **argv)
> int key;
>
> key = IPPROTO_TCP;
> - assert(bpf_map_lookup_elem(map_fd[0], &key, &tcp_cnt) == 0);
> + assert(bpf_map_lookup_elem(map_fd, &key, &tcp_cnt) == 0);
>
> key = IPPROTO_UDP;
> - assert(bpf_map_lookup_elem(map_fd[0], &key, &udp_cnt) == 0);
> + assert(bpf_map_lookup_elem(map_fd, &key, &udp_cnt) == 0);
>
> key = IPPROTO_ICMP;
> - assert(bpf_map_lookup_elem(map_fd[0], &key, &icmp_cnt) == 0);
> + assert(bpf_map_lookup_elem(map_fd, &key, &icmp_cnt) == 0);
>
> printf("TCP %lld UDP %lld ICMP %lld bytes\n",
> tcp_cnt, udp_cnt, icmp_cnt);
> diff --git a/samples/bpf/sockex2_user.c b/samples/bpf/sockex2_user.c
> index 125ee6efc913..e3611dbfce97 100644
> --- a/samples/bpf/sockex2_user.c
> +++ b/samples/bpf/sockex2_user.c
> @@ -3,7 +3,7 @@
> #include <assert.h>
> #include <linux/bpf.h>
> #include <bpf/bpf.h>
> -#include "bpf_load.h"
> +#include "bpf/libbpf.h"
> #include "sock_example.h"
> #include <unistd.h>
> #include <arpa/inet.h>
> @@ -17,22 +17,24 @@ struct pair {
> int main(int ac, char **argv)
> {
> struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
> + struct bpf_object *obj;
> + int map_fd, prog_fd;
> char filename[256];
> - FILE *f;
> int i, sock;
> + FILE *f;
>
> snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> setrlimit(RLIMIT_MEMLOCK, &r);
>
> - if (load_bpf_file(filename)) {
> - printf("%s", bpf_log_buf);
> + if (bpf_prog_load(filename, 0, &obj, &prog_fd))
> return 1;
> - }
> +
> + map_fd = bpf_object__find_map_fd_by_name(obj, "hash_map");
>
> sock = open_raw_sock("lo");
>
> - assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
> - sizeof(prog_fd[0])) == 0);
> + assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd,
> + sizeof(prog_fd)) == 0);
>
> f = popen("ping -4 -c5 localhost", "r");
> (void) f;
> @@ -41,8 +43,8 @@ int main(int ac, char **argv)
> int key = 0, next_key;
> struct pair value;
>
> - while (bpf_map_get_next_key(map_fd[0], &key, &next_key) == 0) {
> - bpf_map_lookup_elem(map_fd[0], &next_key, &value);
> + while (bpf_map_get_next_key(map_fd, &key, &next_key) == 0) {
> + bpf_map_lookup_elem(map_fd, &next_key, &value);
> printf("ip %s bytes %lld packets %lld\n",
> inet_ntoa((struct in_addr){htonl(next_key)}),
> value.bytes, value.packets);
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define for map iteration
From: Andrii Nakryiko @ 2019-02-28 0:09 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227155703.121514a2@cakuba.netronome.com>
On Wed, Feb 27, 2019 at 3:57 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 27 Feb 2019 15:47:56 -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > >
> > > For historical reasons the helper to loop over maps in an object
> > > is called bpf_map__for_each while it really should be called
> > > bpf_object__for_each_map. Rename and add a correctly named
> > > define for backward compatibility.
> >
> > Seems like there are at least 3 more functions that are not named correctly:
> > - __bpf_map__iter (__bpf_object__iter_map?)
> > - bpf_map__next (=> bpf_object__next_map?)
> > - bpf_map__prev (=> bpf_object__prev_map?)
> >
> > Let's rename them as well?
>
> At least those are consistently named between programs and maps.
> I'm happy to do the rename if we don't need backward compat, seems
> a little much to add aliases?
>
> > > Switch all in-tree users to the correct name (Quentin).
> > >
> > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
>
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 6c0168f8bba5..b4652aa1a58a 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -278,10 +278,11 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset);
> > >
> > > LIBBPF_API struct bpf_map *
> > > bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
> > > -#define bpf_map__for_each(pos, obj) \
> > > +#define bpf_object__for_each_map(pos, obj) \
> > > for ((pos) = bpf_map__next(NULL, (obj)); \
> > > (pos) != NULL; \
> > > (pos) = bpf_map__next((pos), (obj)))
> > > +#define bpf_map__for_each bpf_object__for_each_map
> >
> > Should we get rid of this as well, instead of accumulating cruft?
>
> Well, we did some gymnastics in the past to maintain backward compat,
> I thought we do need it..?
I'll let others chime in, but, imo, it feels like while we are at
0.0.2 it might be worthwhile to streamline libbpf early on to reduce
confusion later at the expense of early adopters having to do
straightforward conversion right now.
^ permalink raw reply
* Re: [PATCH bpf-next 5/5] tools: libbpf: make sure readelf shows full names in build checks
From: Andrii Nakryiko @ 2019-02-28 0:11 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-6-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> readelf truncates its output by default to attempt to make it more
> readable. This can lead to function names getting aliased if they
> differ late in the string. Use --wide parameter to avoid
> truncation.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> tools/lib/bpf/Makefile | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index 761691bd72ad..a05c43468bd0 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -132,9 +132,9 @@ BPF_IN := $(OUTPUT)libbpf-in.o
> LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
> VERSION_SCRIPT := libbpf.map
>
> -GLOBAL_SYM_COUNT = $(shell readelf -s $(BPF_IN) | \
> +GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN) | \
> awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {s++} END{print s}')
> -VERSIONED_SYM_COUNT = $(shell readelf -s $(OUTPUT)libbpf.so | \
> +VERSIONED_SYM_COUNT = $(shell readelf -s --wide $(OUTPUT)libbpf.so | \
> grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l)
>
> CMD_TARGETS = $(LIB_FILE)
> --
> 2.19.2
>
Looks good.
Acked-by: Andrii Nakryiko <andriin@fb.com>
^ permalink raw reply
* Re: [PATCH bpf-next 1/5] samples: bpf: force IPv4 in ping
From: Andrii Nakryiko @ 2019-02-28 0:18 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-2-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> ping localhost may default of IPv6 on modern systems, but
> samples are trying to only parse IPv4. Force IPv4.
>
> samples/bpf/tracex1_user.c doesn't interpret the packet so
> we don't care which IP version will be used there.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
> ---
> samples/bpf/sock_example.c | 2 +-
> samples/bpf/sockex1_user.c | 2 +-
> samples/bpf/sockex2_user.c | 2 +-
> samples/bpf/sockex3_user.c | 2 +-
> samples/bpf/tracex2_user.c | 2 +-
> 5 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/samples/bpf/sock_example.c b/samples/bpf/sock_example.c
> index 60ec467c78ab..00aae1d33fca 100644
> --- a/samples/bpf/sock_example.c
> +++ b/samples/bpf/sock_example.c
> @@ -99,7 +99,7 @@ int main(void)
> {
> FILE *f;
>
> - f = popen("ping -c5 localhost", "r");
> + f = popen("ping -4 -c5 localhost", "r");
> (void)f;
>
> return test_sock();
> diff --git a/samples/bpf/sockex1_user.c b/samples/bpf/sockex1_user.c
> index 93ec01c56104..be8ba5686924 100644
> --- a/samples/bpf/sockex1_user.c
> +++ b/samples/bpf/sockex1_user.c
> @@ -26,7 +26,7 @@ int main(int ac, char **argv)
> assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
> sizeof(prog_fd[0])) == 0);
>
> - f = popen("ping -c5 localhost", "r");
> + f = popen("ping -4 -c5 localhost", "r");
> (void) f;
>
> for (i = 0; i < 5; i++) {
> diff --git a/samples/bpf/sockex2_user.c b/samples/bpf/sockex2_user.c
> index 1d5c6e9a6d27..125ee6efc913 100644
> --- a/samples/bpf/sockex2_user.c
> +++ b/samples/bpf/sockex2_user.c
> @@ -34,7 +34,7 @@ int main(int ac, char **argv)
> assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
> sizeof(prog_fd[0])) == 0);
>
> - f = popen("ping -c5 localhost", "r");
> + f = popen("ping -4 -c5 localhost", "r");
> (void) f;
>
> for (i = 0; i < 5; i++) {
> diff --git a/samples/bpf/sockex3_user.c b/samples/bpf/sockex3_user.c
> index 9d02e0404719..bbb1cd0666a9 100644
> --- a/samples/bpf/sockex3_user.c
> +++ b/samples/bpf/sockex3_user.c
> @@ -58,7 +58,7 @@ int main(int argc, char **argv)
> sizeof(__u32)) == 0);
>
> if (argc > 1)
> - f = popen("ping -c5 localhost", "r");
> + f = popen("ping -4 -c5 localhost", "r");
> else
> f = popen("netperf -l 4 localhost", "r");
> (void) f;
> diff --git a/samples/bpf/tracex2_user.c b/samples/bpf/tracex2_user.c
> index 1a81e6a5c2ea..c9544a4ce61a 100644
> --- a/samples/bpf/tracex2_user.c
> +++ b/samples/bpf/tracex2_user.c
> @@ -131,7 +131,7 @@ int main(int ac, char **argv)
> signal(SIGTERM, int_exit);
>
> /* start 'ping' in the background to have some kfree_skb events */
> - f = popen("ping -c5 localhost", "r");
> + f = popen("ping -4 -c5 localhost", "r");
> (void) f;
>
> /* start 'dd' in the background to have plenty of 'write' syscalls */
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Yonghong Song @ 2019-02-28 0:19 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <CAEf4BzbCd9ZWyunwEu78PCK14UA+kR0b7Xp8Lt46Xb2HqUdbQA@mail.gmail.com>
On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
> On Wed, Feb 27, 2019 at 1:23 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Commit d83525ca62cf ("bpf: introduce bpf_spin_lock")
>> introduced bpf_spin_lock and the field spin_lock_off
>> in kernel internal structure bpf_map has the following
>> meaning:
>> >=0 valid offset, <0 error
>>
>> For every map created, the kernel will ensure
>> spin_lock_off has correct value.
>>
>> Currently, bpf_map->spin_lock_off is not copied
>> from the inner map to the map_in_map inner_map_meta
>> during a map_in_map type map creation, so
>> inner_map_meta->spin_lock_off = 0.
>> This will give verifier wrong information that
>> inner_map has bpf_spin_lock and the bpf_spin_lock
>> is defined at offset 0. An access to offset 0
>> of a value pointer will trigger the following error:
>> bpf_spin_lock cannot be accessed directly by load/store
>>
>> This patch fixed the issue by copy inner map's spin_lock_off
>> value to inner_map_meta->spin_lock_off.
>>
>> Fixes: d83525ca62cf ("bpf: introduce bpf_spin_lock")
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> kernel/bpf/map_in_map.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
>> index 583346a0ab29..3dff41403583 100644
>> --- a/kernel/bpf/map_in_map.c
>> +++ b/kernel/bpf/map_in_map.c
>> @@ -58,6 +58,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
>> inner_map_meta->value_size = inner_map->value_size;
>> inner_map_meta->map_flags = inner_map->map_flags;
>> inner_map_meta->max_entries = inner_map->max_entries;
>> + inner_map_meta->spin_lock_off = inner_map->spin_lock_off;
>
> Looks like spinlock inside inner map is not supported: there is
> specific check few lines above returning -ENOSUPP for such case. In
> that case, maybe assign -1 here to make this explicit?
-1 (-EPERM) probably not the best choice. The verifier already has
knowledge that a particular tracked map is an inner map or not. So
keeping the original error code (mostly -EINVAL) is preferred I think.
>
> Though I guess that also brings up the question: is there any harm in
> supporting spin lock for inner map and why it was disabled in the
> first place?
Not exactly sure about the reason. Maybe with this patch, it can get
proper support. Not 100% sure.
>
>>
>> /* Misc members not needed in bpf_map_meta_equal() check. */
>> inner_map_meta->ops = inner_map->ops;
>> --
>> 2.17.1
>>
^ permalink raw reply
* Re: [PATCH bpf-next 2/5] samples: bpf: remove load_sock_ops in favour of bpftool
From: Andrii Nakryiko @ 2019-02-28 0:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-3-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> bpftool can do all the things load_sock_ops used to do, and more.
> Point users to bpftool instead of maintaining this sample utility.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
thanks!
Acked-by: Andrii Nakryiko <andriin@fb.com>
> ---
> samples/bpf/.gitignore | 1 -
> samples/bpf/Makefile | 2 -
> samples/bpf/load_sock_ops.c | 97 ------------------------------
> samples/bpf/tcp_basertt_kern.c | 2 +-
> samples/bpf/tcp_bpf.readme | 14 +++--
> samples/bpf/tcp_bufs_kern.c | 2 +-
> samples/bpf/tcp_clamp_kern.c | 2 +-
> samples/bpf/tcp_cong_kern.c | 2 +-
> samples/bpf/tcp_iw_kern.c | 2 +-
> samples/bpf/tcp_rwnd_kern.c | 2 +-
> samples/bpf/tcp_synrto_kern.c | 2 +-
> samples/bpf/tcp_tos_reflect_kern.c | 2 +-
> 12 files changed, 16 insertions(+), 114 deletions(-)
> delete mode 100644 samples/bpf/load_sock_ops.c
>
> diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore
> index 8ae4940025f8..dbb817dbacfc 100644
> --- a/samples/bpf/.gitignore
> +++ b/samples/bpf/.gitignore
> @@ -1,7 +1,6 @@
> cpustat
> fds_example
> lathist
> -load_sock_ops
> lwt_len_hist
> map_perf_test
> offwaketime
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index a333e258f319..4dd98100678e 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -40,7 +40,6 @@ hostprogs-y += lwt_len_hist
> hostprogs-y += xdp_tx_iptunnel
> hostprogs-y += test_map_in_map
> hostprogs-y += per_socket_stats_example
> -hostprogs-y += load_sock_ops
> hostprogs-y += xdp_redirect
> hostprogs-y += xdp_redirect_map
> hostprogs-y += xdp_redirect_cpu
> @@ -71,7 +70,6 @@ tracex4-objs := bpf_load.o tracex4_user.o
> tracex5-objs := bpf_load.o tracex5_user.o
> tracex6-objs := bpf_load.o tracex6_user.o
> tracex7-objs := bpf_load.o tracex7_user.o
> -load_sock_ops-objs := bpf_load.o load_sock_ops.o
> test_probe_write_user-objs := bpf_load.o test_probe_write_user_user.o
> trace_output-objs := bpf_load.o trace_output_user.o $(TRACE_HELPERS)
> lathist-objs := bpf_load.o lathist_user.o
> diff --git a/samples/bpf/load_sock_ops.c b/samples/bpf/load_sock_ops.c
> deleted file mode 100644
> index 8ecb41ea0c03..000000000000
> --- a/samples/bpf/load_sock_ops.c
> +++ /dev/null
> @@ -1,97 +0,0 @@
> -/* Copyright (c) 2017 Facebook
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of version 2 of the GNU General Public
> - * License as published by the Free Software Foundation.
> - */
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <linux/bpf.h>
> -#include <bpf/bpf.h>
> -#include "bpf_load.h"
> -#include <unistd.h>
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <linux/unistd.h>
> -
> -static void usage(char *pname)
> -{
> - printf("USAGE:\n %s [-l] <cg-path> <prog filename>\n", pname);
> - printf("\tLoad and attach a sock_ops program to the specified "
> - "cgroup\n");
> - printf("\tIf \"-l\" is used, the program will continue to run\n");
> - printf("\tprinting the BPF log buffer\n");
> - printf("\tIf the specified filename does not end in \".o\", it\n");
> - printf("\tappends \"_kern.o\" to the name\n");
> - printf("\n");
> - printf(" %s -r <cg-path>\n", pname);
> - printf("\tDetaches the currently attached sock_ops program\n");
> - printf("\tfrom the specified cgroup\n");
> - printf("\n");
> - exit(1);
> -}
> -
> -int main(int argc, char **argv)
> -{
> - int logFlag = 0;
> - int error = 0;
> - char *cg_path;
> - char fn[500];
> - char *prog;
> - int cg_fd;
> -
> - if (argc < 3)
> - usage(argv[0]);
> -
> - if (!strcmp(argv[1], "-r")) {
> - cg_path = argv[2];
> - cg_fd = open(cg_path, O_DIRECTORY, O_RDONLY);
> - error = bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
> - if (error) {
> - printf("ERROR: bpf_prog_detach: %d (%s)\n",
> - error, strerror(errno));
> - return 2;
> - }
> - return 0;
> - } else if (!strcmp(argv[1], "-h")) {
> - usage(argv[0]);
> - } else if (!strcmp(argv[1], "-l")) {
> - logFlag = 1;
> - if (argc < 4)
> - usage(argv[0]);
> - }
> -
> - prog = argv[argc - 1];
> - cg_path = argv[argc - 2];
> - if (strlen(prog) > 480) {
> - fprintf(stderr, "ERROR: program name too long (> 480 chars)\n");
> - return 3;
> - }
> - cg_fd = open(cg_path, O_DIRECTORY, O_RDONLY);
> -
> - if (!strcmp(prog + strlen(prog)-2, ".o"))
> - strcpy(fn, prog);
> - else
> - sprintf(fn, "%s_kern.o", prog);
> - if (logFlag)
> - printf("loading bpf file:%s\n", fn);
> - if (load_bpf_file(fn)) {
> - printf("ERROR: load_bpf_file failed for: %s\n", fn);
> - printf("%s", bpf_log_buf);
> - return 4;
> - }
> - if (logFlag)
> - printf("TCP BPF Loaded %s\n", fn);
> -
> - error = bpf_prog_attach(prog_fd[0], cg_fd, BPF_CGROUP_SOCK_OPS, 0);
> - if (error) {
> - printf("ERROR: bpf_prog_attach: %d (%s)\n",
> - error, strerror(errno));
> - return 5;
> - } else if (logFlag) {
> - read_trace_pipe();
> - }
> -
> - return error;
> -}
> diff --git a/samples/bpf/tcp_basertt_kern.c b/samples/bpf/tcp_basertt_kern.c
> index 4bf4fc597db9..6ef1625e8b2c 100644
> --- a/samples/bpf/tcp_basertt_kern.c
> +++ b/samples/bpf/tcp_basertt_kern.c
> @@ -7,7 +7,7 @@
> * BPF program to set base_rtt to 80us when host is running TCP-NV and
> * both hosts are in the same datacenter (as determined by IPv6 prefix).
> *
> - * Use load_sock_ops to load this BPF program.
> + * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
> */
>
> #include <uapi/linux/bpf.h>
> diff --git a/samples/bpf/tcp_bpf.readme b/samples/bpf/tcp_bpf.readme
> index 831fb601e3c9..fee746621aec 100644
> --- a/samples/bpf/tcp_bpf.readme
> +++ b/samples/bpf/tcp_bpf.readme
> @@ -8,14 +8,16 @@ a cgroupv2 and attach a bash shell to the group.
> bash
> echo $$ >> /tmp/cgroupv2/foo/cgroup.procs
>
> -Anything that runs under this shell belongs to the foo cgroupv2 To load
> +Anything that runs under this shell belongs to the foo cgroupv2. To load
> (attach) one of the tcp_*_kern.o programs:
>
> - ./load_sock_ops -l /tmp/cgroupv2/foo tcp_basertt_kern.o
> + bpftool prog load tcp_basertt_kern.o /sys/fs/bpf/tcp_prog
> + bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
> + bpftool prog tracelog
>
> -If the "-l" flag is used, the load_sock_ops program will continue to run
> -printing the BPF log buffer. The tcp_*_kern.o programs use special print
> -functions to print logging information (if enabled by the ifdef).
> +"bpftool prog tracelog" will continue to run printing the BPF log buffer.
> +The tcp_*_kern.o programs use special print functions to print logging
> +information (if enabled by the ifdef).
>
> If using netperf/netserver to create traffic, you need to run them under the
> cgroupv2 to which the BPF programs are attached (i.e. under bash shell
> @@ -23,4 +25,4 @@ attached to the cgroupv2).
>
> To remove (unattach) a socket_ops BPF program from a cgroupv2:
>
> - ./load_sock_ops -r /tmp/cgroupv2/foo
> + bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
> diff --git a/samples/bpf/tcp_bufs_kern.c b/samples/bpf/tcp_bufs_kern.c
> index 0566b7fa38a1..e03e204739fa 100644
> --- a/samples/bpf/tcp_bufs_kern.c
> +++ b/samples/bpf/tcp_bufs_kern.c
> @@ -9,7 +9,7 @@
> * doing appropriate checks that indicate the hosts are far enough
> * away (i.e. large RTT).
> *
> - * Use load_sock_ops to load this BPF program.
> + * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
> */
>
> #include <uapi/linux/bpf.h>
> diff --git a/samples/bpf/tcp_clamp_kern.c b/samples/bpf/tcp_clamp_kern.c
> index f4225c9d2c0c..a0dc2d254aca 100644
> --- a/samples/bpf/tcp_clamp_kern.c
> +++ b/samples/bpf/tcp_clamp_kern.c
> @@ -9,7 +9,7 @@
> * the same datacenter. For his example, we assume they are within the same
> * datacenter when the first 5.5 bytes of their IPv6 addresses are the same.
> *
> - * Use load_sock_ops to load this BPF program.
> + * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
> */
>
> #include <uapi/linux/bpf.h>
> diff --git a/samples/bpf/tcp_cong_kern.c b/samples/bpf/tcp_cong_kern.c
> index ad0f1ba8206a..4fd3ca979a06 100644
> --- a/samples/bpf/tcp_cong_kern.c
> +++ b/samples/bpf/tcp_cong_kern.c
> @@ -7,7 +7,7 @@
> * BPF program to set congestion control to dctcp when both hosts are
> * in the same datacenter (as deteremined by IPv6 prefix).
> *
> - * Use load_sock_ops to load this BPF program.
> + * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
> */
>
> #include <uapi/linux/bpf.h>
> diff --git a/samples/bpf/tcp_iw_kern.c b/samples/bpf/tcp_iw_kern.c
> index 4ca5ecc9f580..9b139ec69560 100644
> --- a/samples/bpf/tcp_iw_kern.c
> +++ b/samples/bpf/tcp_iw_kern.c
> @@ -9,7 +9,7 @@
> * would usually be done after doing appropriate checks that indicate
> * the hosts are far enough away (i.e. large RTT).
> *
> - * Use load_sock_ops to load this BPF program.
> + * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
> */
>
> #include <uapi/linux/bpf.h>
> diff --git a/samples/bpf/tcp_rwnd_kern.c b/samples/bpf/tcp_rwnd_kern.c
> index 09ff65b40b31..cc71ee96e044 100644
> --- a/samples/bpf/tcp_rwnd_kern.c
> +++ b/samples/bpf/tcp_rwnd_kern.c
> @@ -8,7 +8,7 @@
> * and the first 5.5 bytes of the IPv6 addresses are not the same (in this
> * example that means both hosts are not the same datacenter).
> *
> - * Use load_sock_ops to load this BPF program.
> + * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
> */
>
> #include <uapi/linux/bpf.h>
> diff --git a/samples/bpf/tcp_synrto_kern.c b/samples/bpf/tcp_synrto_kern.c
> index 232bb242823e..ca87ed34f896 100644
> --- a/samples/bpf/tcp_synrto_kern.c
> +++ b/samples/bpf/tcp_synrto_kern.c
> @@ -8,7 +8,7 @@
> * and the first 5.5 bytes of the IPv6 addresses are the same (in this example
> * that means both hosts are in the same datacenter).
> *
> - * Use load_sock_ops to load this BPF program.
> + * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
> */
>
> #include <uapi/linux/bpf.h>
> diff --git a/samples/bpf/tcp_tos_reflect_kern.c b/samples/bpf/tcp_tos_reflect_kern.c
> index d51dab19eca6..de788be6f862 100644
> --- a/samples/bpf/tcp_tos_reflect_kern.c
> +++ b/samples/bpf/tcp_tos_reflect_kern.c
> @@ -4,7 +4,7 @@
> *
> * BPF program to automatically reflect TOS option from received syn packet
> *
> - * Use load_sock_ops to load this BPF program.
> + * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
> */
>
> #include <uapi/linux/bpf.h>
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH bpf-next 4/5] samples: bpf: use libbpf where easy
From: Jakub Kicinski @ 2019-02-28 0:21 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <CAEf4BzZsYnvQZTqB6g9RocZpuCMxZ+eCnLqBccerFNkQ8ReEFw@mail.gmail.com>
On Wed, 27 Feb 2019 16:05:45 -0800, Andrii Nakryiko wrote:
> > if (object) {
> > - assert(!load_bpf_file((char *)object));
> > - return prog_fd[0];
> > + assert(!bpf_prog_load(object, 0, &obj, &prog_fd));
>
> Here and in few more places below: is it possible to specify correct
> bpf_prog_type instead of 0?
Some of them yes, some of them no. There are objects with multiple
program types IIRC. This program type argument is only used for
checking if kernel versions are needed, and they are provided in
samples so it doesn't matter.
I'll try to add where possible.
^ permalink raw reply
* Re: [PATCH net-next 0/6] net: add individual virtual device filtering
From: Florian Fainelli @ 2019-02-28 0:23 UTC (permalink / raw)
To: Ivan Khoronzhuk, davem, grygorii.strashko
Cc: linux-omap, netdev, linux-kernel, jiri, ilias.apalodimas
In-Reply-To: <20190226184556.16082-1-ivan.khoronzhuk@linaro.org>
Hi Ivan,
On 2/26/19 10:45 AM, Ivan Khoronzhuk wrote:
> One of the reasons of this proposition is safety and performance -
> host should not receive traffic which is not designated for it.
>
> Some network devices can hold separate address tables for vlans and
> real device, but for some reason there is no possibility to apply it
> with generic net addressing scheme easily. At this moment the fastest
> solution is to add mcast/ucast entries for every created vlan
> including real device. But it not only consumes forwarding table but
> also adds holes in the filtering and thus wastes cpus cycles.
>
> This patchseries tries to correct core to assign mcast and ucast
> addresses only for vlans that really require it and as result an end
> driver can exclusively and simply set its rx filters. As an example
> it's implemented on cpsw TI driver, but generic changes provided by
> this series can be reused by other ethernet drivers having similar
> rx filter address possibilities.
>
> An address+vid is considered as separate address. The reserved device
> address length is 32 Bytes, for ethernet devices it's additional
> opportunity to pass auxiliary address info, like virtual ID
> identifying a device the address belongs to. This series makes it
> possible at least for ETH_P_8021Q.
>
> Thus end real device can setup separate tables for virtual devices
> just retrieving VID from the address. A device address space can
> maintain addresses and references on them separately for each virtual
> device if it needs so, or only addresses for real device (and all its
> vlans) it holds usually.
>
> A vlan device can be in any place of device chain upper real device,
> say smth like rdevice/bonding/vlan or even rdevice/macvlan/vlan.
>
> This series is verified on TI am572x EVM that can hold separate tables
> for vlans. Potentially it can be easily extended to netcp driver for
> keystone 2 boards (including k2g) and also new am6 chipsets. As a
> simple test case, different combinations of vlan+macvlan, macvlan+vlan
> were used and tested as with unicast as multicast addresses.
>
> Based on net-next/master
Thanks a lot for posting this patch series, I will take a look later
tonight.
>
> It's continuation of RFC:
>
> [RFC PATCH net-next 0/5] net: allow hw addresses for virtual device
> https://lkml.org/lkml/2018/12/3/817
>
> Ivan Khoronzhuk (6):
> net: core: dev_addr_lists: add VID to device address
> net: 8021q: vlan_dev: add vid tag to addresses of uc and mc lists
> net: 8021q: vlan_dev: add vid tag for vlan device own address
> ethernet: eth: add default vid len for all ehternet kind devices
> net: ethernet: ti: cpsw: update mc filtering to use IVDF
> net: ethernet: ti: cpsw: add macvlan and ucast/vlan filtering support
>
> drivers/net/ethernet/ti/Kconfig | 1 +
> drivers/net/ethernet/ti/cpsw.c | 139 ++++++++++++--------------------
> include/linux/if_vlan.h | 2 +
> include/linux/netdevice.h | 4 +
> net/8021q/Kconfig | 12 +++
> net/8021q/vlan.c | 3 +
> net/8021q/vlan.h | 2 +
> net/8021q/vlan_core.c | 25 ++++++
> net/8021q/vlan_dev.c | 103 ++++++++++++++++++-----
> net/core/dev_addr_lists.c | 124 ++++++++++++++++++++++------
> net/ethernet/eth.c | 10 ++-
> 11 files changed, 292 insertions(+), 133 deletions(-)
>
--
Florian
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Andrii Nakryiko @ 2019-02-28 0:28 UTC (permalink / raw)
To: Yonghong Song
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <e517e58e-a031-189f-658b-0138e3e22cc4@fb.com>
On Wed, Feb 27, 2019 at 4:19 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
> > On Wed, Feb 27, 2019 at 1:23 PM Yonghong Song <yhs@fb.com> wrote:
> >>
> >> Commit d83525ca62cf ("bpf: introduce bpf_spin_lock")
> >> introduced bpf_spin_lock and the field spin_lock_off
> >> in kernel internal structure bpf_map has the following
> >> meaning:
> >> >=0 valid offset, <0 error
> >>
> >> For every map created, the kernel will ensure
> >> spin_lock_off has correct value.
> >>
> >> Currently, bpf_map->spin_lock_off is not copied
> >> from the inner map to the map_in_map inner_map_meta
> >> during a map_in_map type map creation, so
> >> inner_map_meta->spin_lock_off = 0.
> >> This will give verifier wrong information that
> >> inner_map has bpf_spin_lock and the bpf_spin_lock
> >> is defined at offset 0. An access to offset 0
> >> of a value pointer will trigger the following error:
> >> bpf_spin_lock cannot be accessed directly by load/store
> >>
> >> This patch fixed the issue by copy inner map's spin_lock_off
> >> value to inner_map_meta->spin_lock_off.
> >>
> >> Fixes: d83525ca62cf ("bpf: introduce bpf_spin_lock")
> >> Signed-off-by: Yonghong Song <yhs@fb.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
> >> ---
> >> kernel/bpf/map_in_map.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
> >> index 583346a0ab29..3dff41403583 100644
> >> --- a/kernel/bpf/map_in_map.c
> >> +++ b/kernel/bpf/map_in_map.c
> >> @@ -58,6 +58,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
> >> inner_map_meta->value_size = inner_map->value_size;
> >> inner_map_meta->map_flags = inner_map->map_flags;
> >> inner_map_meta->max_entries = inner_map->max_entries;
> >> + inner_map_meta->spin_lock_off = inner_map->spin_lock_off;
> >
> > Looks like spinlock inside inner map is not supported: there is
> > specific check few lines above returning -ENOSUPP for such case. In
> > that case, maybe assign -1 here to make this explicit?
>
> -1 (-EPERM) probably not the best choice. The verifier already has
> knowledge that a particular tracked map is an inner map or not. So
> keeping the original error code (mostly -EINVAL) is preferred I think.
Ah, I actually missed the fact that verifier actually checks those
values (so it's not just >= 0 or < 0), so yeah, let's just pass
through. Btw, the value when there is no spinlock is actually -ENOENT.
>
> >
> > Though I guess that also brings up the question: is there any harm in
> > supporting spin lock for inner map and why it was disabled in the
> > first place?
>
> Not exactly sure about the reason. Maybe with this patch, it can get
> proper support. Not 100% sure.
No, it won't, because bpf_map_meta_alloc explicitly tests for it:
if (map_value_has_spin_lock(inner_map)) {
fdput(f);
return ERR_PTR(-ENOTSUPP);
}
Maybe Alexei can clarify?
>
> >
> >>
> >> /* Misc members not needed in bpf_map_meta_equal() check. */
> >> inner_map_meta->ops = inner_map->ops;
> >> --
> >> 2.17.1
> >>
^ permalink raw reply
* [PATCH net-next] switchdev: Remove unused transaction item queue
From: Florian Fainelli @ 2019-02-28 0:29 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, David S. Miller, Ido Schimmel, open list,
open list:STAGING SUBSYSTEM, moderated list:ETHERNET BRIDGE, jiri,
andrew, vivien.didelot
There are no more in tree users of the
switchdev_trans_item_{dequeue,enqueue} or switchdev_trans_item structure
in the kernel since commit 00fc0c51e35b ("rocker: Change world_ops API
and implementation to be switchdev independant").
Remove this unused code and update the documentation accordingly since.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Documentation/networking/switchdev.txt | 19 -----
include/net/switchdev.h | 12 ---
net/switchdev/switchdev.c | 100 +------------------------
3 files changed, 2 insertions(+), 129 deletions(-)
diff --git a/Documentation/networking/switchdev.txt b/Documentation/networking/switchdev.txt
index 633dd1fd81b7..86174ce8cd13 100644
--- a/Documentation/networking/switchdev.txt
+++ b/Documentation/networking/switchdev.txt
@@ -371,22 +371,3 @@ The driver can monitor for updates to arp_tbl using the netevent notifier
NETEVENT_NEIGH_UPDATE. The device can be programmed with resolved nexthops
for the routes as arp_tbl updates. The driver implements ndo_neigh_destroy
to know when arp_tbl neighbor entries are purged from the port.
-
-Transaction item queue
-^^^^^^^^^^^^^^^^^^^^^^
-
-For switchdev ops attr_set and obj_add, there is a 2 phase transaction model
-used. First phase is to "prepare" anything needed, including various checks,
-memory allocation, etc. The goal is to handle the stuff that is not unlikely
-to fail here. The second phase is to "commit" the actual changes.
-
-Switchdev provides an infrastructure for sharing items (for example memory
-allocations) between the two phases.
-
-The object created by a driver in "prepare" phase and it is queued up by:
-switchdev_trans_item_enqueue()
-During the "commit" phase, the driver gets the object by:
-switchdev_trans_item_dequeue()
-
-If a transaction is aborted during "prepare" phase, switchdev code will handle
-cleanup of the queued-up objects.
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index e4f751e19ecf..0ebd67ae7012 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -20,14 +20,7 @@
#define SWITCHDEV_F_SKIP_EOPNOTSUPP BIT(1)
#define SWITCHDEV_F_DEFER BIT(2)
-struct switchdev_trans_item {
- struct list_head list;
- void *data;
- void (*destructor)(const void *data);
-};
-
struct switchdev_trans {
- struct list_head item_list;
bool ph_prepare;
};
@@ -105,11 +98,6 @@ struct switchdev_obj_port_mdb {
#define SWITCHDEV_OBJ_PORT_MDB(OBJ) \
container_of((OBJ), struct switchdev_obj_port_mdb, obj)
-void switchdev_trans_item_enqueue(struct switchdev_trans *trans,
- void *data, void (*destructor)(void const *),
- struct switchdev_trans_item *tritem);
-void *switchdev_trans_item_dequeue(struct switchdev_trans *trans);
-
typedef int switchdev_obj_dump_cb_t(struct switchdev_obj *obj);
enum switchdev_notifier_type {
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index d81cfcee9ad9..90ba4a1f0a6d 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -23,78 +23,6 @@
#include <linux/rtnetlink.h>
#include <net/switchdev.h>
-/**
- * switchdev_trans_item_enqueue - Enqueue data item to transaction queue
- *
- * @trans: transaction
- * @data: pointer to data being queued
- * @destructor: data destructor
- * @tritem: transaction item being queued
- *
- * Enqeueue data item to transaction queue. tritem is typically placed in
- * cointainter pointed at by data pointer. Destructor is called on
- * transaction abort and after successful commit phase in case
- * the caller did not dequeue the item before.
- */
-void switchdev_trans_item_enqueue(struct switchdev_trans *trans,
- void *data, void (*destructor)(void const *),
- struct switchdev_trans_item *tritem)
-{
- tritem->data = data;
- tritem->destructor = destructor;
- list_add_tail(&tritem->list, &trans->item_list);
-}
-EXPORT_SYMBOL_GPL(switchdev_trans_item_enqueue);
-
-static struct switchdev_trans_item *
-__switchdev_trans_item_dequeue(struct switchdev_trans *trans)
-{
- struct switchdev_trans_item *tritem;
-
- if (list_empty(&trans->item_list))
- return NULL;
- tritem = list_first_entry(&trans->item_list,
- struct switchdev_trans_item, list);
- list_del(&tritem->list);
- return tritem;
-}
-
-/**
- * switchdev_trans_item_dequeue - Dequeue data item from transaction queue
- *
- * @trans: transaction
- */
-void *switchdev_trans_item_dequeue(struct switchdev_trans *trans)
-{
- struct switchdev_trans_item *tritem;
-
- tritem = __switchdev_trans_item_dequeue(trans);
- BUG_ON(!tritem);
- return tritem->data;
-}
-EXPORT_SYMBOL_GPL(switchdev_trans_item_dequeue);
-
-static void switchdev_trans_init(struct switchdev_trans *trans)
-{
- INIT_LIST_HEAD(&trans->item_list);
-}
-
-static void switchdev_trans_items_destroy(struct switchdev_trans *trans)
-{
- struct switchdev_trans_item *tritem;
-
- while ((tritem = __switchdev_trans_item_dequeue(trans)))
- tritem->destructor(tritem->data);
-}
-
-static void switchdev_trans_items_warn_destroy(struct net_device *dev,
- struct switchdev_trans *trans)
-{
- WARN(!list_empty(&trans->item_list), "%s: transaction item queue is not empty.\n",
- dev->name);
- switchdev_trans_items_destroy(trans);
-}
-
static LIST_HEAD(deferred);
static DEFINE_SPINLOCK(deferred_lock);
@@ -208,8 +136,6 @@ static int switchdev_port_attr_set_now(struct net_device *dev,
struct switchdev_trans trans;
int err;
- switchdev_trans_init(&trans);
-
/* Phase I: prepare for attr set. Driver/device should fail
* here if there are going to be issues in the commit phase,
* such as lack of resources or support. The driver/device
@@ -220,17 +146,8 @@ static int switchdev_port_attr_set_now(struct net_device *dev,
trans.ph_prepare = true;
err = switchdev_port_attr_notify(SWITCHDEV_PORT_ATTR_SET, dev, attr,
&trans);
- if (err) {
- /* Prepare phase failed: abort the transaction. Any
- * resources reserved in the prepare phase are
- * released.
- */
-
- if (err != -EOPNOTSUPP)
- switchdev_trans_items_destroy(&trans);
-
+ if (err)
return err;
- }
/* Phase II: commit attr set. This cannot fail as a fault
* of driver/device. If it does, it's a bug in the driver/device
@@ -242,7 +159,6 @@ static int switchdev_port_attr_set_now(struct net_device *dev,
&trans);
WARN(err, "%s: Commit of attribute (id=%d) failed.\n",
dev->name, attr->id);
- switchdev_trans_items_warn_destroy(dev, &trans);
return err;
}
@@ -341,8 +257,6 @@ static int switchdev_port_obj_add_now(struct net_device *dev,
ASSERT_RTNL();
- switchdev_trans_init(&trans);
-
/* Phase I: prepare for obj add. Driver/device should fail
* here if there are going to be issues in the commit phase,
* such as lack of resources or support. The driver/device
@@ -353,17 +267,8 @@ static int switchdev_port_obj_add_now(struct net_device *dev,
trans.ph_prepare = true;
err = switchdev_port_obj_notify(SWITCHDEV_PORT_OBJ_ADD,
dev, obj, &trans, extack);
- if (err) {
- /* Prepare phase failed: abort the transaction. Any
- * resources reserved in the prepare phase are
- * released.
- */
-
- if (err != -EOPNOTSUPP)
- switchdev_trans_items_destroy(&trans);
-
+ if (err)
return err;
- }
/* Phase II: commit obj add. This cannot fail as a fault
* of driver/device. If it does, it's a bug in the driver/device
@@ -374,7 +279,6 @@ static int switchdev_port_obj_add_now(struct net_device *dev,
err = switchdev_port_obj_notify(SWITCHDEV_PORT_OBJ_ADD,
dev, obj, &trans, extack);
WARN(err, "%s: Commit of object (id=%d) failed.\n", dev->name, obj->id);
- switchdev_trans_items_warn_destroy(dev, &trans);
return err;
}
--
2.17.1
^ permalink raw reply related
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: si-wei liu @ 2019-02-28 0:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Samudrala, Sridhar, Siwei Liu, Jiri Pirko, Stephen Hemminger,
David Miller, Netdev, virtualization, virtio-dev,
Brandeburg, Jesse, Alexander Duyck, Jakub Kicinski, Jason Wang,
liran.alon
In-Reply-To: <20190227184601-mutt-send-email-mst@kernel.org>
On 2/27/2019 3:50 PM, Michael S. Tsirkin wrote:
> On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
>>
>> On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
>>> On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
>>>> On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
>>>>> On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
>>>>>> On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
>>>>>>>> On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
>>>>>>>>> On 2/21/2019 7:33 PM, si-wei liu wrote:
>>>>>>>>>> On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
>>>>>>>>>>> On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
>>>>>>>>>>>> Sorry for replying to this ancient thread. There was some remaining
>>>>>>>>>>>> issue that I don't think the initial net_failover patch got addressed
>>>>>>>>>>>> cleanly, see:
>>>>>>>>>>>>
>>>>>>>>>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
>>>>>>>>>>>>
>>>>>>>>>>>> The renaming of 'eth0' to 'ens4' fails because the udev userspace was
>>>>>>>>>>>> not specifically writtten for such kernel automatic enslavement.
>>>>>>>>>>>> Specifically, if it is a bond or team, the slave would typically get
>>>>>>>>>>>> renamed *before* virtual device gets created, that's what udev can
>>>>>>>>>>>> control (without getting netdev opened early by the other part of
>>>>>>>>>>>> kernel) and other userspace components for e.g. initramfs,
>>>>>>>>>>>> init-scripts can coordinate well in between. The in-kernel
>>>>>>>>>>>> auto-enslavement of net_failover breaks this userspace convention,
>>>>>>>>>>>> which don't provides a solution if user care about consistent naming
>>>>>>>>>>>> on the slave netdevs specifically.
>>>>>>>>>>>>
>>>>>>>>>>>> Previously this issue had been specifically called out when IFF_HIDDEN
>>>>>>>>>>>> and the 1-netdev was proposed, but no one gives out a solution to this
>>>>>>>>>>>> problem ever since. Please share your mind how to proceed and solve
>>>>>>>>>>>> this userspace issue if netdev does not welcome a 1-netdev model.
>>>>>>>>>>> Above says:
>>>>>>>>>>>
>>>>>>>>>>> there's no motivation in the systemd/udevd community at
>>>>>>>>>>> this point to refactor the rename logic and make it work well with
>>>>>>>>>>> 3-netdev.
>>>>>>>>>>>
>>>>>>>>>>> What would the fix be? Skip slave devices?
>>>>>>>>>>>
>>>>>>>>>> There's nothing user can get if just skipping slave devices - the
>>>>>>>>>> name is still unchanged and unpredictable e.g. eth0, or eth1 the
>>>>>>>>>> next reboot, while the rest may conform to the naming scheme (ens3
>>>>>>>>>> and such). There's no way one can fix this in userspace alone - when
>>>>>>>>>> the failover is created the enslaved netdev was opened by the kernel
>>>>>>>>>> earlier than the userspace is made aware of, and there's no
>>>>>>>>>> negotiation protocol for kernel to know when userspace has done
>>>>>>>>>> initial renaming of the interface. I would expect netdev list should
>>>>>>>>>> at least provide the direction in general for how this can be
>>>>>>>>>> solved...
>>>>>>> I was just wondering what did you mean when you said
>>>>>>> "refactor the rename logic and make it work well with 3-netdev" -
>>>>>>> was there a proposal udev rejected?
>>>>>> No. I never believed this particular issue can be fixed in userspace alone.
>>>>>> Previously someone had said it could be, but I never see any work or
>>>>>> relevant discussion ever happened in various userspace communities (for e.g.
>>>>>> dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
>>>>>> of the issue derives from the kernel, it makes more sense to start from
>>>>>> netdev, work out and decide on a solution: see what can be done in the
>>>>>> kernel in order to fix it, then after that engage userspace community for
>>>>>> the feasibility...
>>>>>>
>>>>>>> Anyway, can we write a time diagram for what happens in which order that
>>>>>>> leads to failure? That would help look for triggers that we can tie
>>>>>>> into, or add new ones.
>>>>>>>
>>>>>> See attached diagram.
>>>>>>
>>>>>>>
>>>>>>>>> Is there an issue if slave device names are not predictable? The user/admin scripts are expected
>>>>>>>>> to only work with the master failover device.
>>>>>>>> Where does this expectation come from?
>>>>>>>>
>>>>>>>> Admin users may have ethtool or tc configurations that need to deal with
>>>>>>>> predictable interface name. Third-party app which was built upon specifying
>>>>>>>> certain interface name can't be modified to chase dynamic names.
>>>>>>>>
>>>>>>>> Specifically, we have pre-canned image that uses ethtool to fine tune VF
>>>>>>>> offload settings post boot for specific workload. Those images won't work
>>>>>>>> well if the name is constantly changing just after couple rounds of live
>>>>>>>> migration.
>>>>>>> It should be possible to specify the ethtool configuration on the
>>>>>>> master and have it automatically propagated to the slave.
>>>>>>>
>>>>>>> BTW this is something we should look at IMHO.
>>>>>> I was elaborating a few examples that the expectation and assumption that
>>>>>> user/admin scripts only deal with master failover device is incorrect. It
>>>>>> had never been taken good care of, although I did try to emphasize it from
>>>>>> the very beginning.
>>>>>>
>>>>>> Basically what you said about propagating the ethtool configuration down to
>>>>>> the slave is the key pursuance of 1-netdev model. However, what I am seeking
>>>>>> now is any alternative that can also fix the specific udev rename problem,
>>>>>> before concluding that 1-netdev is the only solution. Generally a 1-netdev
>>>>>> scheme would take time to implement, while I'm trying to find a way out to
>>>>>> fix this particular naming problem under 3-netdev.
>>>>>>
>>>>>>>>> Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
>>>>>>>>> about moving them to a hidden network namespace so that they are not visible from the default namespace.
>>>>>>>>> I looked into this sometime back, but did not find the right kernel api to create a network namespace within
>>>>>>>>> kernel. If so, we could use this mechanism to simulate a 1-netdev model.
>>>>>>>> Yes, that's one possible implementation (IMHO the key is to make 1-netdev
>>>>>>>> model as much transparent to a real NIC as possible, while a hidden netns is
>>>>>>>> just the vehicle). However, I recall there was resistance around this
>>>>>>>> discussion that even the concept of hiding itself is a taboo for Linux
>>>>>>>> netdev. I would like to summon potential alternatives before concluding
>>>>>>>> 1-netdev is the only solution too soon.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> -Siwei
>>>>>>> Your scripts would not work at all then, right?
>>>>>> At this point we don't claim images with such usage as SR-IOV live
>>>>>> migrate-able. We would flag it as live migrate-able until this ethtool
>>>>>> config issue is fully addressed and a transparent live migration solution
>>>>>> emerges in upstream eventually.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> -Siwei
>>>>>>>>>> -Siwei
>>>>>>>>>>
>>>>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>>>>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>>>>>>
>>>>>> net_failover(kernel) | network.service (user) | systemd-udevd (user)
>>>>>> --------------------------------------------------+------------------------------+--------------------------------------------
>>>>>> (standby virtio-net and net_failover | |
>>>>>> devices created and initialized, | |
>>>>>> i.e. virtnet_probe()-> | |
>>>>>> net_failover_create() | |
>>>>>> was done.) | |
>>>>>> | |
>>>>>> | runs `ifup ens3' -> |
>>>>>> | ip link set dev ens3 up |
>>>>>> net_failover_open() | |
>>>>>> dev_open(virtnet_dev) | |
>>>>>> virtnet_open(virtnet_dev) | |
>>>>>> netif_carrier_on(failover_dev) | |
>>>>>> ... | |
>>>>>> | |
>>>>>> (VF hot plugged in) | |
>>>>>> ixgbevf_probe() | |
>>>>>> register_netdev(ixgbevf_netdev) | |
>>>>>> netdev_register_kobject(ixgbevf_netdev) | |
>>>>>> kobject_add(ixgbevf_dev) | |
>>>>>> device_add(ixgbevf_dev) | |
>>>>>> kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
>>>>>> netlink_broadcast() | |
>>>>>> ... | |
>>>>>> call_netdevice_notifiers(NETDEV_REGISTER) | |
>>>>>> failover_event(..., NETDEV_REGISTER, ...) | |
>>>>>> failover_slave_register(ixgbevf_netdev) | |
>>>>>> net_failover_slave_register(ixgbevf_netdev) | |
>>>>>> dev_open(ixgbevf_netdev) | |
>>>>>> | |
>>>>>> | |
>>>>>> | | received ADD uevent from netlink fd
>>>>>> | | ...
>>>>>> | | udev-builtin-net_id.c:dev_pci_slot()
>>>>>> | | (decided to renamed 'eth0' )
>>>>>> | | ip link set dev eth0 name ens4
>>>>>> (dev_change_name() returns -EBUSY as | |
>>>>>> ixgbevf_netdev->flags has IFF_UP) | |
>>>>>> | |
>>>>>>
>>>>> Given renaming slaves does not work anyway:
>>>> I was actually thinking what if we relieve the rename restriction just for
>>>> the failover slave? What the impact would be? I think users don't care about
>>>> slave being renamed when it's in use, especially the initial rename.
>>>> Thoughts?
>>>>
>>>>> would it work if we just
>>>>> hard-coded slave names instead?
>>>>>
>>>>> E.g.
>>>>> 1. fail slave renames
>>>>> 2. rename of failover to XX automatically renames standby to XXnsby
>>>>> and primary to XXnpry
>>>> That wouldn't help. The time when the failover master gets renamed, the VF
>>>> may not be present.
>>> In this scheme if VF is not there it will be renamed immediately after registration.
>> Who will be responsible to rename the slave, the kernel?
> That's the idea.
>
>> Note the master's
>> name may or may not come from the userspace. If it comes from the userspace,
>> should the userspace daemon change their expectation not to name/rename
>> _any_ slaves (today there's no distinction)?
> Yes the idea would be to fail renaming slaves.
No I was asking about the userspace expectation: whether it should track
and detect the lifecycle events of failover slaves and decide what to
do. How does it get back to the user specified name if VF is not
enslaved (say someone unloads the virtio-net module)?
As this scheme adds much complexity to the kernel naming convention
(currently it's just ethX names) that no userspace can understand. Will
the change break userspace further?
-Siwei
>
>> How do users know which name to
>> trust, depending on which wins the race more often? Say if kernel wants a
>> ens3npry name while userspace wants it named as ens4.
>>
>> -Siwei
> With this approach kernel will deny attempts by userspace to rename
> slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> will rename both slaves.
>
> It seems pretty solid to me, the only issue is that in theory userspace
> can use a name like XXXnsby for something else. But this seems unlikely.
>
>
>>>> I don't like the idea to delay exposing failover master
>>>> until VF is hot plugged in (probably subject to various failures) later.
>>>>
>>>> Thanks,
>>>> -Siwei
>>> I agree, this was not what I meant.
>>>
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Michael S. Tsirkin @ 2019-02-28 0:38 UTC (permalink / raw)
To: Stephen Hemminger
Cc: si-wei liu, Samudrala, Sridhar, Siwei Liu, Jiri Pirko,
David Miller, Netdev, virtualization, virtio-dev,
Brandeburg, Jesse, Alexander Duyck, Jakub Kicinski, Jason Wang,
liran.alon
In-Reply-To: <20190227160342.788dc2b4@shemminger-XPS-13-9360>
On Wed, Feb 27, 2019 at 04:03:42PM -0800, Stephen Hemminger wrote:
> > With this approach kernel will deny attempts by userspace to rename
> > slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> > will rename both slaves.
> >
> > It seems pretty solid to me, the only issue is that in theory userspace
> > can use a name like XXXnsby for something else. But this seems unlikely.
>
> Similar schemes (with kernel providing naming) were also previously rejected
> upstream.
Links?
I'm inclined to try and see what happens.
> It has been a consistent theme that the kernel should not be in
> the renaming business.
In this case it's not in renaming business per se. The only reason
we even have the original name is due to the ways internal APIs
work. You can look at it as simply having slaves names being
part of master.
> It will certainly break userspace.
That's a strong claim. What is it based on? It so happens that
userspace renaming slaves is already broken on virtio. So we can fix it
any way we like :)
And yes it won't help netvsc because netvsc wants compatibility with old
scripts but then netvsc uses a 2 device model anyway.
--
MST
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Yonghong Song @ 2019-02-28 0:40 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <CAEf4BzZE8W7_VnoPTBftMUjuhVOTFmfWZS8kihPFjqZ+tP5N6w@mail.gmail.com>
On 2/27/19 4:28 PM, Andrii Nakryiko wrote:
> On Wed, Feb 27, 2019 at 4:19 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
>>> On Wed, Feb 27, 2019 at 1:23 PM Yonghong Song <yhs@fb.com> wrote:
>>>>
>>>> Commit d83525ca62cf ("bpf: introduce bpf_spin_lock")
>>>> introduced bpf_spin_lock and the field spin_lock_off
>>>> in kernel internal structure bpf_map has the following
>>>> meaning:
>>>> >=0 valid offset, <0 error
>>>>
>>>> For every map created, the kernel will ensure
>>>> spin_lock_off has correct value.
>>>>
>>>> Currently, bpf_map->spin_lock_off is not copied
>>>> from the inner map to the map_in_map inner_map_meta
>>>> during a map_in_map type map creation, so
>>>> inner_map_meta->spin_lock_off = 0.
>>>> This will give verifier wrong information that
>>>> inner_map has bpf_spin_lock and the bpf_spin_lock
>>>> is defined at offset 0. An access to offset 0
>>>> of a value pointer will trigger the following error:
>>>> bpf_spin_lock cannot be accessed directly by load/store
>>>>
>>>> This patch fixed the issue by copy inner map's spin_lock_off
>>>> value to inner_map_meta->spin_lock_off.
>>>>
>>>> Fixes: d83525ca62cf ("bpf: introduce bpf_spin_lock")
>>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
>>>> ---
>>>> kernel/bpf/map_in_map.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
>>>> index 583346a0ab29..3dff41403583 100644
>>>> --- a/kernel/bpf/map_in_map.c
>>>> +++ b/kernel/bpf/map_in_map.c
>>>> @@ -58,6 +58,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
>>>> inner_map_meta->value_size = inner_map->value_size;
>>>> inner_map_meta->map_flags = inner_map->map_flags;
>>>> inner_map_meta->max_entries = inner_map->max_entries;
>>>> + inner_map_meta->spin_lock_off = inner_map->spin_lock_off;
>>>
>>> Looks like spinlock inside inner map is not supported: there is
>>> specific check few lines above returning -ENOSUPP for such case. In
>>> that case, maybe assign -1 here to make this explicit?
>>
>> -1 (-EPERM) probably not the best choice. The verifier already has
>> knowledge that a particular tracked map is an inner map or not. So
>> keeping the original error code (mostly -EINVAL) is preferred I think.
>
> Ah, I actually missed the fact that verifier actually checks those
> values (so it's not just >= 0 or < 0), so yeah, let's just pass
> through. Btw, the value when there is no spinlock is actually -ENOENT.
If there is no BTF, it will be -EINVAL. If there is BTF and no spinlock
member, mostly -ENOENT.
>
>>
>>>
>>> Though I guess that also brings up the question: is there any harm in
>>> supporting spin lock for inner map and why it was disabled in the
>>> first place?
>>
>> Not exactly sure about the reason. Maybe with this patch, it can get
>> proper support. Not 100% sure.
>
> No, it won't, because bpf_map_meta_alloc explicitly tests for it:
>
> if (map_value_has_spin_lock(inner_map)) {
> fdput(f);
> return ERR_PTR(-ENOTSUPP);
> }
I mean that this can be removed after my patch and it may work :-)
>
> Maybe Alexei can clarify?
>
>
>>
>>>
>>>>
>>>> /* Misc members not needed in bpf_map_meta_equal() check. */
>>>> inner_map_meta->ops = inner_map->ops;
>>>> --
>>>> 2.17.1
>>>>
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Michael S. Tsirkin @ 2019-02-28 0:41 UTC (permalink / raw)
To: si-wei liu
Cc: Samudrala, Sridhar, Siwei Liu, Jiri Pirko, Stephen Hemminger,
David Miller, Netdev, virtualization, virtio-dev,
Brandeburg, Jesse, Alexander Duyck, Jakub Kicinski, Jason Wang,
liran.alon
In-Reply-To: <a617ce13-4114-469d-ef33-a1c91150eeca@oracle.com>
On Wed, Feb 27, 2019 at 04:38:00PM -0800, si-wei liu wrote:
>
>
> On 2/27/2019 3:50 PM, Michael S. Tsirkin wrote:
> > On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
> > >
> > > On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
> > > > On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
> > > > > On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
> > > > > > On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
> > > > > > > On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
> > > > > > > > On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
> > > > > > > > > On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> > > > > > > > > > On 2/21/2019 7:33 PM, si-wei liu wrote:
> > > > > > > > > > > On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> > > > > > > > > > > > On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> > > > > > > > > > > > > Sorry for replying to this ancient thread. There was some remaining
> > > > > > > > > > > > > issue that I don't think the initial net_failover patch got addressed
> > > > > > > > > > > > > cleanly, see:
> > > > > > > > > > > > >
> > > > > > > > > > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> > > > > > > > > > > > >
> > > > > > > > > > > > > The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> > > > > > > > > > > > > not specifically writtten for such kernel automatic enslavement.
> > > > > > > > > > > > > Specifically, if it is a bond or team, the slave would typically get
> > > > > > > > > > > > > renamed *before* virtual device gets created, that's what udev can
> > > > > > > > > > > > > control (without getting netdev opened early by the other part of
> > > > > > > > > > > > > kernel) and other userspace components for e.g. initramfs,
> > > > > > > > > > > > > init-scripts can coordinate well in between. The in-kernel
> > > > > > > > > > > > > auto-enslavement of net_failover breaks this userspace convention,
> > > > > > > > > > > > > which don't provides a solution if user care about consistent naming
> > > > > > > > > > > > > on the slave netdevs specifically.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Previously this issue had been specifically called out when IFF_HIDDEN
> > > > > > > > > > > > > and the 1-netdev was proposed, but no one gives out a solution to this
> > > > > > > > > > > > > problem ever since. Please share your mind how to proceed and solve
> > > > > > > > > > > > > this userspace issue if netdev does not welcome a 1-netdev model.
> > > > > > > > > > > > Above says:
> > > > > > > > > > > >
> > > > > > > > > > > > there's no motivation in the systemd/udevd community at
> > > > > > > > > > > > this point to refactor the rename logic and make it work well with
> > > > > > > > > > > > 3-netdev.
> > > > > > > > > > > >
> > > > > > > > > > > > What would the fix be? Skip slave devices?
> > > > > > > > > > > >
> > > > > > > > > > > There's nothing user can get if just skipping slave devices - the
> > > > > > > > > > > name is still unchanged and unpredictable e.g. eth0, or eth1 the
> > > > > > > > > > > next reboot, while the rest may conform to the naming scheme (ens3
> > > > > > > > > > > and such). There's no way one can fix this in userspace alone - when
> > > > > > > > > > > the failover is created the enslaved netdev was opened by the kernel
> > > > > > > > > > > earlier than the userspace is made aware of, and there's no
> > > > > > > > > > > negotiation protocol for kernel to know when userspace has done
> > > > > > > > > > > initial renaming of the interface. I would expect netdev list should
> > > > > > > > > > > at least provide the direction in general for how this can be
> > > > > > > > > > > solved...
> > > > > > > > I was just wondering what did you mean when you said
> > > > > > > > "refactor the rename logic and make it work well with 3-netdev" -
> > > > > > > > was there a proposal udev rejected?
> > > > > > > No. I never believed this particular issue can be fixed in userspace alone.
> > > > > > > Previously someone had said it could be, but I never see any work or
> > > > > > > relevant discussion ever happened in various userspace communities (for e.g.
> > > > > > > dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
> > > > > > > of the issue derives from the kernel, it makes more sense to start from
> > > > > > > netdev, work out and decide on a solution: see what can be done in the
> > > > > > > kernel in order to fix it, then after that engage userspace community for
> > > > > > > the feasibility...
> > > > > > >
> > > > > > > > Anyway, can we write a time diagram for what happens in which order that
> > > > > > > > leads to failure? That would help look for triggers that we can tie
> > > > > > > > into, or add new ones.
> > > > > > > >
> > > > > > > See attached diagram.
> > > > > > >
> > > > > > > >
> > > > > > > > > > Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> > > > > > > > > > to only work with the master failover device.
> > > > > > > > > Where does this expectation come from?
> > > > > > > > >
> > > > > > > > > Admin users may have ethtool or tc configurations that need to deal with
> > > > > > > > > predictable interface name. Third-party app which was built upon specifying
> > > > > > > > > certain interface name can't be modified to chase dynamic names.
> > > > > > > > >
> > > > > > > > > Specifically, we have pre-canned image that uses ethtool to fine tune VF
> > > > > > > > > offload settings post boot for specific workload. Those images won't work
> > > > > > > > > well if the name is constantly changing just after couple rounds of live
> > > > > > > > > migration.
> > > > > > > > It should be possible to specify the ethtool configuration on the
> > > > > > > > master and have it automatically propagated to the slave.
> > > > > > > >
> > > > > > > > BTW this is something we should look at IMHO.
> > > > > > > I was elaborating a few examples that the expectation and assumption that
> > > > > > > user/admin scripts only deal with master failover device is incorrect. It
> > > > > > > had never been taken good care of, although I did try to emphasize it from
> > > > > > > the very beginning.
> > > > > > >
> > > > > > > Basically what you said about propagating the ethtool configuration down to
> > > > > > > the slave is the key pursuance of 1-netdev model. However, what I am seeking
> > > > > > > now is any alternative that can also fix the specific udev rename problem,
> > > > > > > before concluding that 1-netdev is the only solution. Generally a 1-netdev
> > > > > > > scheme would take time to implement, while I'm trying to find a way out to
> > > > > > > fix this particular naming problem under 3-netdev.
> > > > > > >
> > > > > > > > > > Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> > > > > > > > > > about moving them to a hidden network namespace so that they are not visible from the default namespace.
> > > > > > > > > > I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> > > > > > > > > > kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> > > > > > > > > Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> > > > > > > > > model as much transparent to a real NIC as possible, while a hidden netns is
> > > > > > > > > just the vehicle). However, I recall there was resistance around this
> > > > > > > > > discussion that even the concept of hiding itself is a taboo for Linux
> > > > > > > > > netdev. I would like to summon potential alternatives before concluding
> > > > > > > > > 1-netdev is the only solution too soon.
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > -Siwei
> > > > > > > > Your scripts would not work at all then, right?
> > > > > > > At this point we don't claim images with such usage as SR-IOV live
> > > > > > > migrate-able. We would flag it as live migrate-able until this ethtool
> > > > > > > config issue is fully addressed and a transparent live migration solution
> > > > > > > emerges in upstream eventually.
> > > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > -Siwei
> > > > > > > > > > > -Siwei
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > ---------------------------------------------------------------------
> > > > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > > > >
> > > > > > > net_failover(kernel) | network.service (user) | systemd-udevd (user)
> > > > > > > --------------------------------------------------+------------------------------+--------------------------------------------
> > > > > > > (standby virtio-net and net_failover | |
> > > > > > > devices created and initialized, | |
> > > > > > > i.e. virtnet_probe()-> | |
> > > > > > > net_failover_create() | |
> > > > > > > was done.) | |
> > > > > > > | |
> > > > > > > | runs `ifup ens3' -> |
> > > > > > > | ip link set dev ens3 up |
> > > > > > > net_failover_open() | |
> > > > > > > dev_open(virtnet_dev) | |
> > > > > > > virtnet_open(virtnet_dev) | |
> > > > > > > netif_carrier_on(failover_dev) | |
> > > > > > > ... | |
> > > > > > > | |
> > > > > > > (VF hot plugged in) | |
> > > > > > > ixgbevf_probe() | |
> > > > > > > register_netdev(ixgbevf_netdev) | |
> > > > > > > netdev_register_kobject(ixgbevf_netdev) | |
> > > > > > > kobject_add(ixgbevf_dev) | |
> > > > > > > device_add(ixgbevf_dev) | |
> > > > > > > kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
> > > > > > > netlink_broadcast() | |
> > > > > > > ... | |
> > > > > > > call_netdevice_notifiers(NETDEV_REGISTER) | |
> > > > > > > failover_event(..., NETDEV_REGISTER, ...) | |
> > > > > > > failover_slave_register(ixgbevf_netdev) | |
> > > > > > > net_failover_slave_register(ixgbevf_netdev) | |
> > > > > > > dev_open(ixgbevf_netdev) | |
> > > > > > > | |
> > > > > > > | |
> > > > > > > | | received ADD uevent from netlink fd
> > > > > > > | | ...
> > > > > > > | | udev-builtin-net_id.c:dev_pci_slot()
> > > > > > > | | (decided to renamed 'eth0' )
> > > > > > > | | ip link set dev eth0 name ens4
> > > > > > > (dev_change_name() returns -EBUSY as | |
> > > > > > > ixgbevf_netdev->flags has IFF_UP) | |
> > > > > > > | |
> > > > > > >
> > > > > > Given renaming slaves does not work anyway:
> > > > > I was actually thinking what if we relieve the rename restriction just for
> > > > > the failover slave? What the impact would be? I think users don't care about
> > > > > slave being renamed when it's in use, especially the initial rename.
> > > > > Thoughts?
> > > > >
> > > > > > would it work if we just
> > > > > > hard-coded slave names instead?
> > > > > >
> > > > > > E.g.
> > > > > > 1. fail slave renames
> > > > > > 2. rename of failover to XX automatically renames standby to XXnsby
> > > > > > and primary to XXnpry
> > > > > That wouldn't help. The time when the failover master gets renamed, the VF
> > > > > may not be present.
> > > > In this scheme if VF is not there it will be renamed immediately after registration.
> > > Who will be responsible to rename the slave, the kernel?
> > That's the idea.
> >
> > > Note the master's
> > > name may or may not come from the userspace. If it comes from the userspace,
> > > should the userspace daemon change their expectation not to name/rename
> > > _any_ slaves (today there's no distinction)?
> > Yes the idea would be to fail renaming slaves.
> No I was asking about the userspace expectation: whether it should track and
> detect the lifecycle events of failover slaves and decide what to do. How
> does it get back to the user specified name if VF is not enslaved (say
> someone unloads the virtio-net module)?
When virtio net is removed VF will shortly be removed too.
> As this scheme adds much complexity to the kernel naming convention
> (currently it's just ethX names) that no userspace can understand.
Anything that pokes at slaves needs to be specially designed anyway.
Naming seems like a minor issue.
> Will the
> change break userspace further?
>
> -Siwei
Didn't you show userspace is already broken. You can't "further
break it", rename already fails.
> >
> > > How do users know which name to
> > > trust, depending on which wins the race more often? Say if kernel wants a
> > > ens3npry name while userspace wants it named as ens4.
> > >
> > > -Siwei
> > With this approach kernel will deny attempts by userspace to rename
> > slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> > will rename both slaves.
> >
> > It seems pretty solid to me, the only issue is that in theory userspace
> > can use a name like XXXnsby for something else. But this seems unlikely.
> >
> >
> > > > > I don't like the idea to delay exposing failover master
> > > > > until VF is hot plugged in (probably subject to various failures) later.
> > > > >
> > > > > Thanks,
> > > > > -Siwei
> > > > I agree, this was not what I meant.
> > > >
^ permalink raw reply
* Re: [PATCH 3/8] tlan: use pci_zalloc instead of pci_alloc
From: Robert Eshleman @ 2019-02-28 0:41 UTC (permalink / raw)
To: Joe Perches
Cc: Tariq Toukan, David S. Miller, Samuel Chessman, netdev,
linux-rdma, linux-kernel
In-Reply-To: <ce11feb21863662cf2ad96f0f7b69f0edcabfcf7.camel@perches.com>
On Tue, Feb 26, 2019 at 10:22:06PM -0800, Joe Perches wrote:
> On Tue, 2019-02-26 at 22:09 -0800, Robert Eshleman wrote:
> > This patch replaces a pci_alloc_consistent and memset(,0) call
> > with a single call to pci_zalloc_consistent.
> []
> > diff --git a/drivers/net/ethernet/ti/tlan.c b/drivers/net/ethernet/ti/tlan.c
> []
> > @@ -845,17 +845,16 @@ static int tlan_init(struct net_device *dev)
> >
> > dma_size = (TLAN_NUM_RX_LISTS + TLAN_NUM_TX_LISTS)
> > * (sizeof(struct tlan_list));
> > - priv->dma_storage = pci_alloc_consistent(priv->pci_dev,
> > - dma_size,
> > - &priv->dma_storage_dma);
> > + priv->dma_storage = pci_zalloc_consistent(priv->pci_dev,
> > + dma_size,
> > + &priv->dma_storage_dma);
> > priv->dma_size = dma_size;
> >
> > - if (priv->dma_storage == NULL) {
> > + if (!priv->dma_storage) {
> > pr_err("Could not allocate lists and buffers for %s\n",
> > dev->name);
>
> unrelated trivia:
>
> This pr_err (and likely others in this file)
> could be replace by netdev_err
>
Definitely good to know (I may make that change too). Thanks Joe.
-Bobby
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox