* Re: [PATCH bpf-next v2 0/8] bpf: TCP RTT sock_ops bpf callback
From: Y Song @ 2019-07-02 17:55 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, bpf, David Miller, Alexei Starovoitov, Daniel Borkmann,
Eric Dumazet, Priyaranjan Jha, Yuchung Cheng,
Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
On Tue, Jul 2, 2019 at 9:14 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Congestion control team would like to have a periodic callback to
> track some TCP statistics. Let's add a sock_ops callback that can be
> selectively enabled on a socket by socket basis and is executed for
> every RTT. BPF program frequency can be further controlled by calling
> bpf_ktime_get_ns and bailing out early.
>
> I run neper tcp_stream and tcp_rr tests with the sample program
> from the last patch and didn't observe any noticeable performance
> difference.
>
> v2:
> * add a comment about second accept() in selftest (Yonghong Song)
> * refer to tcp_bpf.readme in sample program (Yonghong Song)
>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Priyaranjan Jha <priyarjha@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
> Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
> Acked-by: Yuchung Cheng <ycheng@google.com>
Ack for the whole series.
Acked-by: Yonghong Song <yhs@fb.com>
>
> Stanislav Fomichev (8):
> bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
> bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
> bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
> bpf: add icsk_retransmits to bpf_tcp_sock
> bpf/tools: sync bpf.h
> selftests/bpf: test BPF_SOCK_OPS_RTT_CB
> samples/bpf: add sample program that periodically dumps TCP stats
> samples/bpf: fix tcp_bpf.readme detach command
>
> include/net/tcp.h | 8 +
> include/uapi/linux/bpf.h | 12 +-
> net/core/filter.c | 207 +++++++++++-----
> net/ipv4/tcp_input.c | 4 +
> samples/bpf/Makefile | 1 +
> samples/bpf/tcp_bpf.readme | 2 +-
> samples/bpf/tcp_dumpstats_kern.c | 68 ++++++
> tools/include/uapi/linux/bpf.h | 12 +-
> tools/testing/selftests/bpf/Makefile | 3 +-
> tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
> tools/testing/selftests/bpf/test_tcp_rtt.c | 254 ++++++++++++++++++++
> 11 files changed, 574 insertions(+), 58 deletions(-)
> create mode 100644 samples/bpf/tcp_dumpstats_kern.c
> create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
> create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Y Song @ 2019-07-02 17:54 UTC (permalink / raw)
To: Ilya Leoshkevich; +Cc: bpf, netdev
In-Reply-To: <20190702153908.41562-1-iii@linux.ibm.com>
On Tue, Jul 2, 2019 at 8:40 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Use PT_REGS_RC(ctx) instead of ctx->rax, which is not present on s390.
>
> Pass -D__TARGET_ARCH_$(ARCH) to selftests in order to choose a proper
> PT_REGS_RC variant.
>
> Fix s930 -> s390 typo.
>
> On s390, provide the forward declaration of struct pt_regs and cast it
> to user_pt_regs in PT_REGS_* macros. This is necessary, because instead
> of the full struct pt_regs, s390 exposes only its first field
> user_pt_regs to userspace, and bpf_helpers.h is used with both userspace
> (in selftests) and kernel (in samples) headers.
>
> On x86, provide userspace versions of PT_REGS_* macros. Unlike s390, x86
> provides struct pt_regs to both userspace and kernel, however, with
> different field names.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> tools/testing/selftests/bpf/Makefile | 4 +-
> tools/testing/selftests/bpf/bpf_helpers.h | 46 +++++++++++++++--------
> tools/testing/selftests/bpf/progs/loop1.c | 2 +-
> tools/testing/selftests/bpf/progs/loop2.c | 2 +-
> tools/testing/selftests/bpf/progs/loop3.c | 2 +-
> 5 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index d60fee59fbd1..599b320bef65 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> include ../../../../scripts/Kbuild.include
> +include ../../../scripts/Makefile.arch
>
> LIBDIR := ../../../lib
> BPFDIR := $(LIBDIR)/bpf
> @@ -138,7 +139,8 @@ CLANG_SYS_INCLUDES := $(shell $(CLANG) -v -E - </dev/null 2>&1 \
>
> CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
> $(CLANG_SYS_INCLUDES) \
> - -Wno-compare-distinct-pointer-types
> + -Wno-compare-distinct-pointer-types \
> + -D__TARGET_ARCH_$(ARCH)
>
> $(OUTPUT)/test_l4lb_noinline.o: CLANG_FLAGS += -fno-inline
> $(OUTPUT)/test_xdp_noinline.o: CLANG_FLAGS += -fno-inline
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index 1a5b1accf091..faf86d83301a 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -312,8 +312,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #if defined(__TARGET_ARCH_x86)
> #define bpf_target_x86
> #define bpf_target_defined
> -#elif defined(__TARGET_ARCH_s930x)
> - #define bpf_target_s930x
> +#elif defined(__TARGET_ARCH_s390)
> + #define bpf_target_s390
> #define bpf_target_defined
> #elif defined(__TARGET_ARCH_arm)
> #define bpf_target_arm
> @@ -338,8 +338,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #ifndef bpf_target_defined
> #if defined(__x86_64__)
> #define bpf_target_x86
> -#elif defined(__s390x__)
> - #define bpf_target_s930x
> +#elif defined(__s390__)
> + #define bpf_target_s390
> #elif defined(__arm__)
> #define bpf_target_arm
> #elif defined(__aarch64__)
> @@ -355,6 +355,7 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
>
> #if defined(bpf_target_x86)
>
> +#ifdef __KERNEL__
> #define PT_REGS_PARM1(x) ((x)->di)
> #define PT_REGS_PARM2(x) ((x)->si)
> #define PT_REGS_PARM3(x) ((x)->dx)
> @@ -365,19 +366,34 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #define PT_REGS_RC(x) ((x)->ax)
> #define PT_REGS_SP(x) ((x)->sp)
> #define PT_REGS_IP(x) ((x)->ip)
> +#else
> +#define PT_REGS_PARM1(x) ((x)->rdi)
> +#define PT_REGS_PARM2(x) ((x)->rsi)
> +#define PT_REGS_PARM3(x) ((x)->rdx)
> +#define PT_REGS_PARM4(x) ((x)->rcx)
> +#define PT_REGS_PARM5(x) ((x)->r8)
> +#define PT_REGS_RET(x) ((x)->rsp)
> +#define PT_REGS_FP(x) ((x)->rbp)
> +#define PT_REGS_RC(x) ((x)->rax)
> +#define PT_REGS_SP(x) ((x)->rsp)
> +#define PT_REGS_IP(x) ((x)->rip)
> +#endif
>
> -#elif defined(bpf_target_s390x)
> +#elif defined(bpf_target_s390)
>
> -#define PT_REGS_PARM1(x) ((x)->gprs[2])
> -#define PT_REGS_PARM2(x) ((x)->gprs[3])
> -#define PT_REGS_PARM3(x) ((x)->gprs[4])
> -#define PT_REGS_PARM4(x) ((x)->gprs[5])
> -#define PT_REGS_PARM5(x) ((x)->gprs[6])
> -#define PT_REGS_RET(x) ((x)->gprs[14])
> -#define PT_REGS_FP(x) ((x)->gprs[11]) /* Works only with CONFIG_FRAME_POINTER */
> -#define PT_REGS_RC(x) ((x)->gprs[2])
> -#define PT_REGS_SP(x) ((x)->gprs[15])
> -#define PT_REGS_IP(x) ((x)->psw.addr)
> +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
> +struct pt_regs;
> +#define PT_REGS_PARM1(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> +#define PT_REGS_PARM2(x) (((const volatile user_pt_regs *)(x))->gprs[3])
> +#define PT_REGS_PARM3(x) (((const volatile user_pt_regs *)(x))->gprs[4])
> +#define PT_REGS_PARM4(x) (((const volatile user_pt_regs *)(x))->gprs[5])
> +#define PT_REGS_PARM5(x) (((const volatile user_pt_regs *)(x))->gprs[6])
> +#define PT_REGS_RET(x) (((const volatile user_pt_regs *)(x))->gprs[14])
> +/* Works only with CONFIG_FRAME_POINTER */
> +#define PT_REGS_FP(x) (((const volatile user_pt_regs *)(x))->gprs[11])
> +#define PT_REGS_RC(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> +#define PT_REGS_SP(x) (((const volatile user_pt_regs *)(x))->gprs[15])
> +#define PT_REGS_IP(x) (((const volatile user_pt_regs *)(x))->psw.addr)
>
> #elif defined(bpf_target_arm)
>
> diff --git a/tools/testing/selftests/bpf/progs/loop1.c b/tools/testing/selftests/bpf/progs/loop1.c
> index dea395af9ea9..7cdb7f878310 100644
> --- a/tools/testing/selftests/bpf/progs/loop1.c
> +++ b/tools/testing/selftests/bpf/progs/loop1.c
> @@ -18,7 +18,7 @@ int nested_loops(volatile struct pt_regs* ctx)
> for (j = 0; j < 300; j++)
> for (i = 0; i < j; i++) {
> if (j & 1)
> - m = ctx->rax;
> + m = PT_REGS_RC(ctx);
> else
> m = j;
> sum += i * m;
> diff --git a/tools/testing/selftests/bpf/progs/loop2.c b/tools/testing/selftests/bpf/progs/loop2.c
> index 0637bd8e8bcf..9b2f808a2863 100644
> --- a/tools/testing/selftests/bpf/progs/loop2.c
> +++ b/tools/testing/selftests/bpf/progs/loop2.c
> @@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
> int i = 0;
>
> while (true) {
> - if (ctx->rax & 1)
> + if (PT_REGS_RC(ctx) & 1)
> i += 3;
> else
> i += 7;
> diff --git a/tools/testing/selftests/bpf/progs/loop3.c b/tools/testing/selftests/bpf/progs/loop3.c
> index 30a0f6cba080..d727657d51e2 100644
> --- a/tools/testing/selftests/bpf/progs/loop3.c
> +++ b/tools/testing/selftests/bpf/progs/loop3.c
> @@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
> __u64 i = 0, sum = 0;
> do {
> i++;
> - sum += ctx->rax;
> + sum += PT_REGS_RC(ctx);
> } while (i < 0x100000000ULL);
> return sum;
> }
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Y Song @ 2019-07-02 17:53 UTC (permalink / raw)
To: Ilya Leoshkevich; +Cc: bpf, netdev
In-Reply-To: <1AE29825-8FB2-4682-8822-5F3D16965657@linux.ibm.com>
On Tue, Jul 2, 2019 at 9:58 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> > Am 02.07.2019 um 18:42 schrieb Y Song <ys114321@gmail.com>:
> >
> > On Tue, Jul 2, 2019 at 8:40 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >>
> >> -#elif defined(__s390x__)
> >> - #define bpf_target_s930x
> >
> > I see in some other places (e.g., bcc) where
> > macro __s390x__ is also used to indicate a s390 architecture.
> > Could you explain the difference between __s390__ and
> > __s390x__?
>
> __s390__ is defined for 32-bit and 64-bit variants, __s390x__ is defined
> for 64-bit variant only.
Thanks.
>
> >> #if defined(bpf_target_x86)
> >>
> >> +#ifdef __KERNEL__
> >
> > In samples/bpf/, __KERNEL__ is defined at clang options and
> > in selftests/bpf/, the __KERNEL__ is not defined.
> >
> > I checked x86 pt_regs definition with and without __KERNEL__.
> > They are identical except some register name difference.
> > I am wondering whether we can unify into all without
> > __KERNEL__. Is __KERNEL__ really needed?
>
> Right now removing it causes the build to fail, but the errors look
> fixable. However, I wonder whether there is a plan regarding this:
> should eBPF programs be built with user headers, kernel headers,
> or both? Status quo appears to be "both", so I’ve decided to stick with
> that in this patch.
Your patch is okay in the sense it maintains the current behavor.
I think it is okay since user level and kernel pt_regs layout are the same
except certain names are different.
>
> >> +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
> >> +struct pt_regs;
> >> +#define PT_REGS_PARM1(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> >
> > Is user_pt_regs a recent change or has been there for quite some time?
> > I am asking since bcc did not use user_pt_regs yet.
>
> It was added in late 2017 in commit 466698e654e8 ("s390/bpf: correct
> broken uapi for BPF_PROG_TYPE_PERF_EVENT program type“).
Thanks.
^ permalink raw reply
* Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute
From: Jakub Kicinski @ 2019-07-02 17:47 UTC (permalink / raw)
To: Parav Pandit; +Cc: Jiri Pirko, netdev@vger.kernel.org, Saeed Mahameed
In-Reply-To: <AM0PR05MB4866085BC8B082EFD5B59DD2D1F80@AM0PR05MB4866.eurprd05.prod.outlook.com>
On Tue, 2 Jul 2019 04:26:47 +0000, Parav Pandit wrote:
> > On Mon, 1 Jul 2019 07:27:32 -0500, Parav Pandit wrote:
> > > In an eswitch, PCI PF may have port which is normally represented
> > > using a representor netdevice.
> > > To have better visibility of eswitch port, its association with PF, a
> > > representor netdevice and port number, introduce a PCI PF port flavour
> > > and port attriute.
> > >
> > > When devlink port flavour is PCI PF, fill up PCI PF attributes of the
> > > port.
> > >
> > > Extend port name creation using PCI PF number on best effort basis.
> > > So that vendor drivers can skip defining their own scheme.
> > >
> > > $ devlink port show
> > > pci/0000:05:00.0/0: type eth netdev eth0 flavour pcipf pfnum 0
> > >
> > > Acked-by: Jiri Pirko <jiri@mellanox.com>
> > > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > > diff --git a/include/net/devlink.h b/include/net/devlink.h index
> > > 6625ea068d5e..8db9c0e83fb5 100644
> > > --- a/include/net/devlink.h
> > > +++ b/include/net/devlink.h
> > > @@ -38,6 +38,10 @@ struct devlink {
> > > char priv[0] __aligned(NETDEV_ALIGN); };
> > >
> > > +struct devlink_port_pci_pf_attrs {
> >
> > Why the named structure? Anonymous one should be just fine?
> >
> No specific reason for this patch. But named structure allows to
> extend it more easily with code readability.
I'd argue the readability - I hove to scroll up/look up the structure
just to see it has a single member. But no big deal :)
> Such as subsequently we want to add the peer_mac etc port attributes.
> Named structure to store those attributes are helpful.
It remains to be seen if peer attributes are flavour specific 🤔
I'd imagine most port types would have some form of a peer (other
than a network port, perhaps). But perhaps different peer attributes.
> > > diff --git a/net/core/devlink.c b/net/core/devlink.c index
> > > 89c533778135..001f9e2c96f0 100644
> > > --- a/net/core/devlink.c
> > > +++ b/net/core/devlink.c
> > > @@ -517,6 +517,11 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
> > > return -EMSGSIZE;
> > > if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs->port_number))
> > > return -EMSGSIZE;
> >
> > Why would we report network port information for PF and VF port
> > flavours?
>
> I didn't see any immediate need to report, at the same time didn't
> find any reason to treat such port flavours differently than existing
> one. It just gives a clear view of the device's eswitch. Might find
> it useful during debugging while inspecting device internal tables..
PFs and VFs ports are not tied to network ports in switchdev mode.
You have only one network port under a devlink instance AFAIR, anyway.
> > > + if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_PF) {
> > > + if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
> > > + attrs->pci_pf.pf))
> > > + return -EMSGSIZE;
> > > + }
> > > if (!attrs->split)
> > > return 0;
> > > if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_GROUP, attrs->port_number))
^ permalink raw reply
* [PATCH net-next v2] bonding: add an option to specify a delay between peer notifications
From: Vincent Bernat @ 2019-07-02 17:43 UTC (permalink / raw)
To: Jiri Pirko, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
David S. Miller, netdev
Cc: Vincent Bernat
In-Reply-To: <20190701092758.GA2250@nanopsycho>
Currently, gratuitous ARP/ND packets are sent every `miimon'
milliseconds. This commit allows a user to specify a custom delay
through a new option, `peer_notif_delay'.
Like for `updelay' and `downdelay', this delay should be a multiple of
`miimon' to avoid managing an additional work queue. The configuration
logic is copied from `updelay' and `downdelay'. However, the default
value cannot be set using a module parameter: Netlink or sysfs should
be used to configure this feature.
When setting `miimon' to 100 and `peer_notif_delay' to 500, we can
observe the 500 ms delay is respected:
20:30:19.354693 ARP, Request who-has 203.0.113.10 tell 203.0.113.10, length 28
20:30:19.874892 ARP, Request who-has 203.0.113.10 tell 203.0.113.10, length 28
20:30:20.394919 ARP, Request who-has 203.0.113.10 tell 203.0.113.10, length 28
20:30:20.914963 ARP, Request who-has 203.0.113.10 tell 203.0.113.10, length 28
In bond_mii_monitor(), I have tried to keep the lock logic readable.
The change is due to the fact we cannot rely on a notification to
lower the value of `bond->send_peer_notif' as `NETDEV_NOTIFY_PEERS' is
only triggered once every N times, while we need to decrement the
counter each time.
iproute2 also needs to be updated to be able to specify this new
attribute through `ip link'.
Signed-off-by: Vincent Bernat <vincent@bernat.ch>
---
drivers/net/bonding/bond_main.c | 31 ++++++++-----
drivers/net/bonding/bond_netlink.c | 14 ++++++
drivers/net/bonding/bond_options.c | 71 +++++++++++++++++++-----------
drivers/net/bonding/bond_procfs.c | 2 +
drivers/net/bonding/bond_sysfs.c | 13 ++++++
include/net/bond_options.h | 1 +
include/net/bonding.h | 1 +
include/uapi/linux/if_link.h | 1 +
tools/include/uapi/linux/if_link.h | 1 +
9 files changed, 98 insertions(+), 37 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4f5b3baf04c3..74e90381a317 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -796,6 +796,8 @@ static bool bond_should_notify_peers(struct bonding *bond)
slave ? slave->dev->name : "NULL");
if (!slave || !bond->send_peer_notif ||
+ bond->send_peer_notif %
+ max(1, bond->params.peer_notif_delay) != 0 ||
!netif_carrier_ok(bond->dev) ||
test_bit(__LINK_STATE_LINKWATCH_PENDING, &slave->dev->state))
return false;
@@ -886,15 +888,18 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
if (netif_running(bond->dev)) {
bond->send_peer_notif =
- bond->params.num_peer_notif;
+ bond->params.num_peer_notif *
+ max(1, bond->params.peer_notif_delay);
should_notify_peers =
bond_should_notify_peers(bond);
}
call_netdevice_notifiers(NETDEV_BONDING_FAILOVER, bond->dev);
- if (should_notify_peers)
+ if (should_notify_peers) {
+ bond->send_peer_notif--;
call_netdevice_notifiers(NETDEV_NOTIFY_PEERS,
bond->dev);
+ }
}
}
@@ -2279,6 +2284,7 @@ static void bond_mii_monitor(struct work_struct *work)
struct bonding *bond = container_of(work, struct bonding,
mii_work.work);
bool should_notify_peers = false;
+ bool commit;
unsigned long delay;
struct slave *slave;
struct list_head *iter;
@@ -2289,12 +2295,19 @@ static void bond_mii_monitor(struct work_struct *work)
goto re_arm;
rcu_read_lock();
-
should_notify_peers = bond_should_notify_peers(bond);
-
- if (bond_miimon_inspect(bond)) {
+ commit = !!bond_miimon_inspect(bond);
+ if (bond->send_peer_notif) {
+ rcu_read_unlock();
+ if (rtnl_trylock()) {
+ bond->send_peer_notif--;
+ rtnl_unlock();
+ }
+ } else {
rcu_read_unlock();
+ }
+ if (commit) {
/* Race avoidance with bond_close cancel of workqueue */
if (!rtnl_trylock()) {
delay = 1;
@@ -2308,8 +2321,7 @@ static void bond_mii_monitor(struct work_struct *work)
bond_miimon_commit(bond);
rtnl_unlock(); /* might sleep, hold no other locks */
- } else
- rcu_read_unlock();
+ }
re_arm:
if (bond->params.miimon)
@@ -3065,10 +3077,6 @@ static int bond_master_netdev_event(unsigned long event,
case NETDEV_REGISTER:
bond_create_proc_entry(event_bond);
break;
- case NETDEV_NOTIFY_PEERS:
- if (event_bond->send_peer_notif)
- event_bond->send_peer_notif--;
- break;
default:
break;
}
@@ -4691,6 +4699,7 @@ static int bond_check_params(struct bond_params *params)
params->arp_all_targets = arp_all_targets_value;
params->updelay = updelay;
params->downdelay = downdelay;
+ params->peer_notif_delay = 0;
params->use_carrier = use_carrier;
params->lacp_fast = lacp_fast;
params->primary[0] = 0;
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index b24cce48ae35..a259860a7208 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -108,6 +108,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
[IFLA_BOND_AD_ACTOR_SYSTEM] = { .type = NLA_BINARY,
.len = ETH_ALEN },
[IFLA_BOND_TLB_DYNAMIC_LB] = { .type = NLA_U8 },
+ [IFLA_BOND_PEER_NOTIF_DELAY] = { .type = NLA_U32 },
};
static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
@@ -215,6 +216,14 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[],
if (err)
return err;
}
+ if (data[IFLA_BOND_PEER_NOTIF_DELAY]) {
+ int delay = nla_get_u32(data[IFLA_BOND_PEER_NOTIF_DELAY]);
+
+ bond_opt_initval(&newval, delay);
+ err = __bond_opt_set(bond, BOND_OPT_PEER_NOTIF_DELAY, &newval);
+ if (err)
+ return err;
+ }
if (data[IFLA_BOND_USE_CARRIER]) {
int use_carrier = nla_get_u8(data[IFLA_BOND_USE_CARRIER]);
@@ -494,6 +503,7 @@ static size_t bond_get_size(const struct net_device *bond_dev)
nla_total_size(sizeof(u16)) + /* IFLA_BOND_AD_USER_PORT_KEY */
nla_total_size(ETH_ALEN) + /* IFLA_BOND_AD_ACTOR_SYSTEM */
nla_total_size(sizeof(u8)) + /* IFLA_BOND_TLB_DYNAMIC_LB */
+ nla_total_size(sizeof(u32)) + /* IFLA_BOND_PEER_NOTIF_DELAY */
0;
}
@@ -536,6 +546,10 @@ static int bond_fill_info(struct sk_buff *skb,
bond->params.downdelay * bond->params.miimon))
goto nla_put_failure;
+ if (nla_put_u32(skb, IFLA_BOND_PEER_NOTIF_DELAY,
+ bond->params.downdelay * bond->params.miimon))
+ goto nla_put_failure;
+
if (nla_put_u8(skb, IFLA_BOND_USE_CARRIER, bond->params.use_carrier))
goto nla_put_failure;
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 0d852fe9da7c..ddb3916d3506 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -24,6 +24,8 @@ static int bond_option_updelay_set(struct bonding *bond,
const struct bond_opt_value *newval);
static int bond_option_downdelay_set(struct bonding *bond,
const struct bond_opt_value *newval);
+static int bond_option_peer_notif_delay_set(struct bonding *bond,
+ const struct bond_opt_value *newval);
static int bond_option_use_carrier_set(struct bonding *bond,
const struct bond_opt_value *newval);
static int bond_option_arp_interval_set(struct bonding *bond,
@@ -424,6 +426,13 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
.desc = "Number of peer notifications to send on failover event",
.values = bond_num_peer_notif_tbl,
.set = bond_option_num_peer_notif_set
+ },
+ [BOND_OPT_PEER_NOTIF_DELAY] = {
+ .id = BOND_OPT_PEER_NOTIF_DELAY,
+ .name = "peer_notif_delay",
+ .desc = "Delay between each peer notification on failover event, in milliseconds",
+ .values = bond_intmax_tbl,
+ .set = bond_option_peer_notif_delay_set
}
};
@@ -841,6 +850,9 @@ static int bond_option_miimon_set(struct bonding *bond,
if (bond->params.downdelay)
netdev_dbg(bond->dev, "Note: Updating downdelay (to %d) since it is a multiple of the miimon value\n",
bond->params.downdelay * bond->params.miimon);
+ if (bond->params.peer_notif_delay)
+ netdev_dbg(bond->dev, "Note: Updating peer_notif_delay (to %d) since it is a multiple of the miimon value\n",
+ bond->params.peer_notif_delay * bond->params.miimon);
if (newval->value && bond->params.arp_interval) {
netdev_dbg(bond->dev, "MII monitoring cannot be used with ARP monitoring - disabling ARP monitoring...\n");
bond->params.arp_interval = 0;
@@ -864,52 +876,59 @@ static int bond_option_miimon_set(struct bonding *bond,
return 0;
}
-/* Set up and down delays. These must be multiples of the
- * MII monitoring value, and are stored internally as the multiplier.
- * Thus, we must translate to MS for the real world.
+/* Set up, down and peer notification delays. These must be multiples
+ * of the MII monitoring value, and are stored internally as the
+ * multiplier. Thus, we must translate to MS for the real world.
*/
-static int bond_option_updelay_set(struct bonding *bond,
- const struct bond_opt_value *newval)
+static int _bond_option_delay_set(struct bonding *bond,
+ const struct bond_opt_value *newval,
+ const char *name,
+ int *target)
{
int value = newval->value;
if (!bond->params.miimon) {
- netdev_err(bond->dev, "Unable to set up delay as MII monitoring is disabled\n");
+ netdev_err(bond->dev, "Unable to set %s as MII monitoring is disabled\n",
+ name);
return -EPERM;
}
if ((value % bond->params.miimon) != 0) {
- netdev_warn(bond->dev, "up delay (%d) is not a multiple of miimon (%d), updelay rounded to %d ms\n",
+ netdev_warn(bond->dev,
+ "%s (%d) is not a multiple of miimon (%d), value rounded to %d ms\n",
+ name,
value, bond->params.miimon,
(value / bond->params.miimon) *
bond->params.miimon);
}
- bond->params.updelay = value / bond->params.miimon;
- netdev_dbg(bond->dev, "Setting up delay to %d\n",
- bond->params.updelay * bond->params.miimon);
+ *target = value / bond->params.miimon;
+ netdev_dbg(bond->dev, "Setting %s to %d\n",
+ name,
+ *target * bond->params.miimon);
return 0;
}
+static int bond_option_updelay_set(struct bonding *bond,
+ const struct bond_opt_value *newval)
+{
+ return _bond_option_delay_set(bond, newval, "up delay",
+ &bond->params.updelay);
+}
+
static int bond_option_downdelay_set(struct bonding *bond,
const struct bond_opt_value *newval)
{
- int value = newval->value;
-
- if (!bond->params.miimon) {
- netdev_err(bond->dev, "Unable to set down delay as MII monitoring is disabled\n");
- return -EPERM;
- }
- if ((value % bond->params.miimon) != 0) {
- netdev_warn(bond->dev, "down delay (%d) is not a multiple of miimon (%d), delay rounded to %d ms\n",
- value, bond->params.miimon,
- (value / bond->params.miimon) *
- bond->params.miimon);
- }
- bond->params.downdelay = value / bond->params.miimon;
- netdev_dbg(bond->dev, "Setting down delay to %d\n",
- bond->params.downdelay * bond->params.miimon);
+ return _bond_option_delay_set(bond, newval, "down delay",
+ &bond->params.downdelay);
+}
- return 0;
+static int bond_option_peer_notif_delay_set(struct bonding *bond,
+ const struct bond_opt_value *newval)
+{
+ int ret = _bond_option_delay_set(bond, newval,
+ "peer notification delay",
+ &bond->params.peer_notif_delay);
+ return ret;
}
static int bond_option_use_carrier_set(struct bonding *bond,
diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index 9f7d83e827c3..fd5c9cbe45b1 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -104,6 +104,8 @@ static void bond_info_show_master(struct seq_file *seq)
bond->params.updelay * bond->params.miimon);
seq_printf(seq, "Down Delay (ms): %d\n",
bond->params.downdelay * bond->params.miimon);
+ seq_printf(seq, "Peer Notification Delay (ms): %d\n",
+ bond->params.peer_notif_delay * bond->params.miimon);
/* ARP information */
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 94214eaf53c5..2d615a93685e 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -327,6 +327,18 @@ static ssize_t bonding_show_updelay(struct device *d,
static DEVICE_ATTR(updelay, 0644,
bonding_show_updelay, bonding_sysfs_store_option);
+static ssize_t bonding_show_peer_notif_delay(struct device *d,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct bonding *bond = to_bond(d);
+
+ return sprintf(buf, "%d\n",
+ bond->params.peer_notif_delay * bond->params.miimon);
+}
+static DEVICE_ATTR(peer_notif_delay, 0644,
+ bonding_show_peer_notif_delay, bonding_sysfs_store_option);
+
/* Show the LACP interval. */
static ssize_t bonding_show_lacp(struct device *d,
struct device_attribute *attr,
@@ -718,6 +730,7 @@ static struct attribute *per_bond_attrs[] = {
&dev_attr_arp_ip_target.attr,
&dev_attr_downdelay.attr,
&dev_attr_updelay.attr,
+ &dev_attr_peer_notif_delay.attr,
&dev_attr_lacp_rate.attr,
&dev_attr_ad_select.attr,
&dev_attr_xmit_hash_policy.attr,
diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index 2a05cc349018..9d382f2f0bc5 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -63,6 +63,7 @@ enum {
BOND_OPT_AD_ACTOR_SYSTEM,
BOND_OPT_AD_USER_PORT_KEY,
BOND_OPT_NUM_PEER_NOTIF_ALIAS,
+ BOND_OPT_PEER_NOTIF_DELAY,
BOND_OPT_LAST
};
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 676e7fae05a3..f7fe45689142 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -123,6 +123,7 @@ struct bond_params {
int fail_over_mac;
int updelay;
int downdelay;
+ int peer_notif_delay;
int lacp_fast;
unsigned int min_links;
int ad_select;
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 6f75bda2c2d7..4a8c02cafa9a 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -636,6 +636,7 @@ enum {
IFLA_BOND_AD_USER_PORT_KEY,
IFLA_BOND_AD_ACTOR_SYSTEM,
IFLA_BOND_TLB_DYNAMIC_LB,
+ IFLA_BOND_PEER_NOTIF_DELAY,
__IFLA_BOND_MAX,
};
diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
index 5b225ff63b48..7d113a9602f0 100644
--- a/tools/include/uapi/linux/if_link.h
+++ b/tools/include/uapi/linux/if_link.h
@@ -636,6 +636,7 @@ enum {
IFLA_BOND_AD_USER_PORT_KEY,
IFLA_BOND_AD_ACTOR_SYSTEM,
IFLA_BOND_TLB_DYNAMIC_LB,
+ IFLA_BOND_PEER_NOTIF_DELAY,
__IFLA_BOND_MAX,
};
--
2.20.1
^ permalink raw reply related
* [PATCH bpf v2] selftests: bpf: fix inlines in test_lwt_seg6local
From: Jiri Benc @ 2019-07-02 17:40 UTC (permalink / raw)
To: netdev, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Mathieu Xhonneux, Song Liu,
Y Song
Selftests are reporting this failure in test_lwt_seg6local.sh:
+ ip netns exec ns2 ip -6 route add fb00::6 encap bpf in obj test_lwt_seg6local.o sec encap_srh dev veth2
Error fetching program/map!
Failed to parse eBPF program: Operation not permitted
The problem is __attribute__((always_inline)) alone is not enough to prevent
clang from inserting those functions in .text. In that case, .text is not
marked as relocateable.
See the output of objdump -h test_lwt_seg6local.o:
Idx Name Size VMA LMA File off Algn
0 .text 00003530 0000000000000000 0000000000000000 00000040 2**3
CONTENTS, ALLOC, LOAD, READONLY, CODE
This causes the iproute bpf loader to fail in bpf_fetch_prog_sec:
bpf_has_call_data returns true but bpf_fetch_prog_relo fails as there's no
relocateable .text section in the file.
To fix this, convert to 'static __always_inline'.
v2: Use 'static __always_inline' instead of 'static inline
__attribute__((always_inline))'
Fixes: c99a84eac026 ("selftests/bpf: test for seg6local End.BPF action")
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
.../testing/selftests/bpf/progs/test_lwt_seg6local.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/test_lwt_seg6local.c b/tools/testing/selftests/bpf/progs/test_lwt_seg6local.c
index 0575751bc1bc..e2f6ed0a583d 100644
--- a/tools/testing/selftests/bpf/progs/test_lwt_seg6local.c
+++ b/tools/testing/selftests/bpf/progs/test_lwt_seg6local.c
@@ -61,7 +61,7 @@ struct sr6_tlv_t {
unsigned char value[0];
} BPF_PACKET_HEADER;
-__attribute__((always_inline)) struct ip6_srh_t *get_srh(struct __sk_buff *skb)
+static __always_inline struct ip6_srh_t *get_srh(struct __sk_buff *skb)
{
void *cursor, *data_end;
struct ip6_srh_t *srh;
@@ -95,7 +95,7 @@ __attribute__((always_inline)) struct ip6_srh_t *get_srh(struct __sk_buff *skb)
return srh;
}
-__attribute__((always_inline))
+static __always_inline
int update_tlv_pad(struct __sk_buff *skb, uint32_t new_pad,
uint32_t old_pad, uint32_t pad_off)
{
@@ -125,7 +125,7 @@ int update_tlv_pad(struct __sk_buff *skb, uint32_t new_pad,
return 0;
}
-__attribute__((always_inline))
+static __always_inline
int is_valid_tlv_boundary(struct __sk_buff *skb, struct ip6_srh_t *srh,
uint32_t *tlv_off, uint32_t *pad_size,
uint32_t *pad_off)
@@ -184,7 +184,7 @@ int is_valid_tlv_boundary(struct __sk_buff *skb, struct ip6_srh_t *srh,
return 0;
}
-__attribute__((always_inline))
+static __always_inline
int add_tlv(struct __sk_buff *skb, struct ip6_srh_t *srh, uint32_t tlv_off,
struct sr6_tlv_t *itlv, uint8_t tlv_size)
{
@@ -228,7 +228,7 @@ int add_tlv(struct __sk_buff *skb, struct ip6_srh_t *srh, uint32_t tlv_off,
return update_tlv_pad(skb, new_pad, pad_size, pad_off);
}
-__attribute__((always_inline))
+static __always_inline
int delete_tlv(struct __sk_buff *skb, struct ip6_srh_t *srh,
uint32_t tlv_off)
{
@@ -266,7 +266,7 @@ int delete_tlv(struct __sk_buff *skb, struct ip6_srh_t *srh,
return update_tlv_pad(skb, new_pad, pad_size, pad_off);
}
-__attribute__((always_inline))
+static __always_inline
int has_egr_tlv(struct __sk_buff *skb, struct ip6_srh_t *srh)
{
int tlv_offset = sizeof(struct ip6_t) + sizeof(struct ip6_srh_t) +
--
2.18.1
^ permalink raw reply related
* Re: [PATCH bpf-next v2] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-07-02 17:31 UTC (permalink / raw)
To: Takshak Chahande; +Cc: netdev, ast, daniel, kernel-team
In-Reply-To: <20190702164843.3662715-1-ctakshak@fb.com>
On Tue, 2 Jul 2019 09:48:43 -0700, Takshak Chahande wrote:
> + cgroup_fd = open(cgroup_path, O_RDONLY);
> if (cgroup_fd < 0) {
> - p_err("can't open cgroup %s", argv[1]);
> + p_err("can't open cgroup %s", cgroup_path);
AFAICS the bpf tree has not been merged into bpf-next yet, and there
will be a conflict here. For no good reason. Please wait, I will
post my patch for inclusion when the time is right.
> goto exit;
> }
^ permalink raw reply
* Re: [PATCH bpf] selftests: bpf: fix inlines in test_lwt_seg6local
From: Jiri Benc @ 2019-07-02 17:29 UTC (permalink / raw)
To: Y Song
Cc: Song Liu, Networking, bpf, Alexei Starovoitov, Daniel Borkmann,
Mathieu Xhonneux
In-Reply-To: <CAH3MdRUbkswKAYiDSmhe9cdd-Jd=YmC0_PSLhzfY7vKv-zxCCA@mail.gmail.com>
On Mon, 1 Jul 2019 11:39:27 -0700, Y Song wrote:
> By default, we have
> # define __always_inline inline __attribute__((always_inline))
>
> So just use __always_inline should be less verbose in your patch.
I'll resubmit, converting everything to __always_inline, targeting
bpf-next.
> BTW, what compiler did you use have this behavior?
clang version 7.0.1 (tags/RELEASE_701/final)
LLVM version 7.0.1
> Did you have issues with `static __attribute__((always_inline))`?
That seems to work, too.
Jiri
^ permalink raw reply
* RE: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the port parameters.
From: Parav Pandit @ 2019-07-02 17:18 UTC (permalink / raw)
To: Andrew Lunn
Cc: Sudarsana Reddy Kalluru, davem@davemloft.net,
netdev@vger.kernel.org, mkalderon@marvell.com, aelior@marvell.com,
jiri@resnulli.us
In-Reply-To: <20190702164844.GA28471@lunn.ch>
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, July 2, 2019 10:19 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Sudarsana Reddy Kalluru <skalluru@marvell.com>; davem@davemloft.net;
> netdev@vger.kernel.org; mkalderon@marvell.com; aelior@marvell.com;
> jiri@resnulli.us
> Subject: Re: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the
> port parameters.
>
> > A vendor driver calling these APIs is needed at minimum.
>
> Not a vendor driver, but a mainline driver.
>
My apologies for terminology.
I meant to say that a NIC/hw driver from the kernel tree that created the devlink port should call this API (as user) in the patch.
You said it rightly below. Thanks.
> But yes, a new API should not be added without at least one user.
>
> Andrew
^ permalink raw reply
* [PATCH] bpf: Replace a seq_printf() call by seq_puts() in btf_enum_seq_show()
From: Markus Elfring @ 2019-07-02 17:13 UTC (permalink / raw)
To: bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 Jul 2019 19:04:08 +0200
A string which did not contain a data format specification should be put
into a sequence. Thus use the corresponding function “seq_puts”.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
kernel/bpf/btf.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 546ebee39e2a..679a19968f29 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -2426,9 +2426,8 @@ static void btf_enum_seq_show(const struct btf *btf, const struct btf_type *t,
for (i = 0; i < nr_enums; i++) {
if (v == enums[i].val) {
- seq_printf(m, "%s",
- __btf_name_by_offset(btf,
- enums[i].name_off));
+ seq_puts(m,
+ __btf_name_by_offset(btf, enums[i].name_off));
return;
}
}
--
2.22.0
^ permalink raw reply related
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Ido Schimmel @ 2019-07-02 17:11 UTC (permalink / raw)
To: Linus Lüssing
Cc: Russell King - ARM Linux admin, nikolay, Ido Schimmel,
Vivien Didelot, Florian Fainelli, netdev@vger.kernel.org,
Jiri Pirko, andrew@lunn.ch, davem@davemloft.net, bridge,
b.a.t.m.a.n
In-Reply-To: <20190630165601.GC2500@otheros>
On Sun, Jun 30, 2019 at 06:56:01PM +0200, Linus Lüssing wrote:
> > On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
> > > See commit b00589af3b04 ("bridge: disable snooping if there is no
> > > querier"). I think that's unfortunate behavior that we need because
> > > multicast snooping is enabled by default. If it weren't enabled by
> > > default, then anyone enabling it would also make sure there's a querier
> > > in the network.
>
> I do not quite understand that point. In a way, that's what we
> have right now, isn't it? By default it's disabled, because by
> default there is no querier on the link. So anyone wanting to use
> multicast snooping will need to make sure there's a querier in the
> network.
Hi Linus,
Querier state is not reflected to drivers ATM, so drivers believe the
bridge is multicast aware and unregistered multicast packets are only
flooded to mrouter ports. Hosts that are silent (because there is no
querier) never get the traffic addressed to them (f.e., IPv6 neighbour
solicitation).
> Overall I think the querier (election) mechanism in the standards could
> need an update. While the lowest-address first might have
> worked well back then, in uniform, fully wired networks where the
> position of the querier did not matter, this is not a good
> solution anymore in networks involving wireless, dynamic connections.
> Especially in wireless mesh networks this is a bit of an issue for
> us. Ideally, the querier mechanism were dismissed in favour of simply
> unsolicited, periodic IGMP/MLD reports...
>
> But of course, updating IETF standards is no solution for now.
>
> While more complicated, it would not be impossible to consider the
> querier state, would it? I mean you probably already need to
> consider the case of a user disabling multicast snooping during
> runtime, right?
Sure, this is implemented.
> So similarly, you could react to appearing or disappearing queriers?
Yes, but it's a bit more complicated since we need to differentiate
between IPv4 and IPv6. If the bridge is multicast aware, but there is
only IPv4 querier on the link, then:
1. All the IPv6 MDB entries need to be removed from the device. At least
in mlxsw, we do not have a way to ignore only IPv6 entries. From the
device's perspective, an MDB entry is just a multicast DMAC with a
bitmap of ports packets should be replicated to.
2. We need to split the flood tables used for IPv4 and IPv6 unregistered
multicast packets. For IPv4, packets should only be flooded to mrouter
ports whereas for IPv6 packets should be flooded to all the member
ports.
Do you differentiate between IPv4 and IPv6 in batman-adv?
> Cheers, Linus
Thanks for the feedback!
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Ilya Leoshkevich @ 2019-07-02 16:58 UTC (permalink / raw)
To: Y Song; +Cc: bpf, netdev
In-Reply-To: <CAH3MdRUk5x2D9yRuKpGpVuDMFF0JbYeB+Y0Qz6chtPgfm-1vxA@mail.gmail.com>
> Am 02.07.2019 um 18:42 schrieb Y Song <ys114321@gmail.com>:
>
> On Tue, Jul 2, 2019 at 8:40 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>
>> -#elif defined(__s390x__)
>> - #define bpf_target_s930x
>
> I see in some other places (e.g., bcc) where
> macro __s390x__ is also used to indicate a s390 architecture.
> Could you explain the difference between __s390__ and
> __s390x__?
__s390__ is defined for 32-bit and 64-bit variants, __s390x__ is defined
for 64-bit variant only.
>> #if defined(bpf_target_x86)
>>
>> +#ifdef __KERNEL__
>
> In samples/bpf/, __KERNEL__ is defined at clang options and
> in selftests/bpf/, the __KERNEL__ is not defined.
>
> I checked x86 pt_regs definition with and without __KERNEL__.
> They are identical except some register name difference.
> I am wondering whether we can unify into all without
> __KERNEL__. Is __KERNEL__ really needed?
Right now removing it causes the build to fail, but the errors look
fixable. However, I wonder whether there is a plan regarding this:
should eBPF programs be built with user headers, kernel headers,
or both? Status quo appears to be "both", so I’ve decided to stick with
that in this patch.
>> +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
>> +struct pt_regs;
>> +#define PT_REGS_PARM1(x) (((const volatile user_pt_regs *)(x))->gprs[2])
>
> Is user_pt_regs a recent change or has been there for quite some time?
> I am asking since bcc did not use user_pt_regs yet.
It was added in late 2017 in commit 466698e654e8 ("s390/bpf: correct
broken uapi for BPF_PROG_TYPE_PERF_EVENT program type“).
^ permalink raw reply
* Re: [PATCH net-next] r8169: add random MAC address fallback
From: Heiner Kallweit @ 2019-07-02 16:49 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <61a7754f-bdf9-f69a-296d-47353a78c8b4@gmail.com>
On 02.07.2019 08:18, Heiner Kallweit wrote:
>>From 1c8bacf724f1450e5256c68fbff407305faf9cbd Mon Sep 17 00:00:00 2001
>
>
>
Sorry, something went wrong when preparing the commit message. I'll resubmit.
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/ethernet/realtek/r8169_main.c | 40 +++++++++++++++--------
> 1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 450c74dc1..d6c137b7f 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -6651,13 +6651,36 @@ static int rtl_get_ether_clk(struct rtl8169_private *tp)
> return rc;
> }
>
> +static void rtl_init_mac_address(struct rtl8169_private *tp)
> +{
> + struct net_device *dev = tp->dev;
> + u8 *mac_addr = dev->dev_addr;
> + int rc, i;
> +
> + rc = eth_platform_get_mac_address(tp_to_dev(tp), mac_addr);
> + if (!rc)
> + goto done;
> +
> + rtl_read_mac_address(tp, mac_addr);
> + if (is_valid_ether_addr(mac_addr))
> + goto done;
> +
> + for (i = 0; i < ETH_ALEN; i++)
> + mac_addr[i] = RTL_R8(tp, MAC0 + i);
> + if (is_valid_ether_addr(mac_addr))
> + goto done;
> +
> + eth_hw_addr_random(dev);
> + dev_warn(tp_to_dev(tp), "can't read MAC address, setting random one\n");
> +done:
> + rtl_rar_set(tp, mac_addr);
> +}
> +
> static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> - /* align to u16 for is_valid_ether_addr() */
> - u8 mac_addr[ETH_ALEN] __aligned(2) = {};
> struct rtl8169_private *tp;
> struct net_device *dev;
> - int chipset, region, i;
> + int chipset, region;
> int jumbo_max, rc;
>
> dev = devm_alloc_etherdev(&pdev->dev, sizeof (*tp));
> @@ -6749,16 +6772,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> u64_stats_init(&tp->rx_stats.syncp);
> u64_stats_init(&tp->tx_stats.syncp);
>
> - /* get MAC address */
> - rc = eth_platform_get_mac_address(&pdev->dev, mac_addr);
> - if (rc)
> - rtl_read_mac_address(tp, mac_addr);
> -
> - if (is_valid_ether_addr(mac_addr))
> - rtl_rar_set(tp, mac_addr);
> -
> - for (i = 0; i < ETH_ALEN; i++)
> - dev->dev_addr[i] = RTL_R8(tp, MAC0 + i);
> + rtl_init_mac_address(tp);
>
> dev->ethtool_ops = &rtl8169_ethtool_ops;
>
>
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: santosh.shilimkar @ 2019-07-02 16:49 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <6ff00a46-07f6-7be2-8e75-c87448568aa4@oracle.com>
On 7/1/19 10:11 PM, Gerd Rausch wrote:
> Hi Santosh,
>
> On 01/07/2019 19.28, santosh.shilimkar@oracle.com wrote:
>>>
>> Below. All command timeouts are 60 seconds.
>>
>> enum {
>> MLX4_CMD_TIME_CLASS_A = 60000,
>> MLX4_CMD_TIME_CLASS_B = 60000,
>> MLX4_CMD_TIME_CLASS_C = 60000,
>> };
>>
>
> Thank you for the pointer.
>
>> But having said that, I re-looked the code you are patching
>> and thats actually only FRWR code which is purely work-request
>> based so this command timeout shouldn't matter.
>>
>
> Which brings us back full circle to the question of
> what the timeout ought to be?
>
> Please keep in mind that prior to this fix,
> the RDS code didn't wait at all:
>
> It simply posted those registration (IB_WR_REG_MR)
> and invalidation (IB_WR_LOCAL_INV)
> work-requests, with no regards to when the firmware
> would execute them.
>
> Arguably, waiting any amount time greater than zero
> for the operation to complete is better than not waiting at all.
>
> We can change the timeout to a high value, or even make it infinite
> by using "wait_event" instead of "wait_event_timeout".
>
> For the registration work-requests there is a benefit to wait a short
> amount of time only (the trade-off described in patch #1 of this series).
>
Actually we should just switch this code to what Avinash has
finally made in downstream code. That keeps the RDS_GET_MR
semantics and makes sure MR is really valid before handing over
the key to userland. There is no need for any timeout
for registration case.
> For de-registration work-requests, it is beneficial to wait
> until they are truly done.
> But: Function "rds_ib_unreg_frmr" prior and post this change
> simply moves on after a failed de-registration attempt,
> and releases the pages owned by the memory region.
>
> This patch does _not_ change that behavior.
>
>> If the work request fails, then it will lead to flush errors and
>> MRs will be marked as STALE. So this wait may not be necessary
>>
>
> This wait is necessary to avoid the 2 scenarios described
> in the commit-log message:
>
> #1) Memory regions bouncing between "drop_list" and "clean_list"
> as items on the "clean_list" aren't really clean until
> their state transitions to "FRMR_IS_FREE".
>
> #2) Prevent an access error as "rds_ib_post_inv" is called
> just prior to de-referencing pages via "__rds_ib_teardown_mr".
> And you certainly don't want those pages populated in the
> HCA's memory-translation-table with full access, while
> the Linux kernel 'thinks' you gave them back already
> and starts re-purposing them.
>
>> RDS_GET_MR case is what actually showing the issue you saw
>> and the fix for that Avinash has it in production kernel.
>
> Actually, no:
> Socket option RDS_GET_MR wasn't even in the code-path of the
> tests I performed:
>
> It were there RDS_CMSG_RDMA_MAP / RDS_CMSG_RDMA_DEST control
> messages that ended up calling '__rds_rdma_map".
>
What option did you use ? Default option with rds-stress is
RDS_GET_MR and hence the question.
>>
>> I believe with that change, registration issue becomes non-issue
>> already.
>>
>
> Please explain how that is related to this fix-suggestion?
>
> I submitted this patch #3 and the others in this series in order
> to fix bugs in the RDS that is currently shipped with Linux.
>
> It may very well be the case that there are other changes
> that Avinash put into production kernels that would be better
> suited to fix this and other problems.
>
> But that should not eliminate the need to fix what is currently broken.
>
> Fixing what's broken does not preclude replacing the fixed code
> with newer or better versions of the same.
>
>> And as far as invalidation concerned with proxy qp, it not longer
>> races with data path qp.
>>
>
> I don't understand, please elaborate.
>
>> May be you can try those changes if not already to see if it
>> addresses the couple of cases where you ended up adding
>> timeouts.
>>
>
> I don't understand, please elaborate:
> a) Are you saying this issue should not be fixed?
> b) Or are you suggesting to replace this fix with a different fix?
> If it's the later, please point out what you have in mind.
> c) ???
>
All am saying is the code got changed for good reason and that changed
code makes some of these race conditions possibly not applicable.
So instead of these timeout fixes, am suggesting to use that
code as fix. At least test it with those changes and see whats
the behavior.
Regards,
Santosh
^ permalink raw reply
* [PATCH bpf-next v2] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Takshak Chahande @ 2019-07-02 16:48 UTC (permalink / raw)
To: netdev; +Cc: ast, jakub.kicinski, daniel, ctakshak, kernel-team
With different bpf attach_flags available to attach bpf programs specially
with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective
bpf-programs available to any sub-cgroups really needs to be available for
easy debugging.
Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached
bpf-programs to a cgroup but also the inherited ones from parent cgroup.
So additional "effective" option is introduced to use BPF_F_QUERY_EFFECTIVE
query flag here to list all the effective bpf-programs available for execution
at a specified cgroup.
Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf:
# ./test_cgroup_attach
With old bpftool (without 'effective' option)
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
ID AttachType AttachFlags Name
129 egress multi pkt_cntr_1
130 egress multi pkt_cntr_2
Attached new program pkt_cntr_4 in cg2 gives following:
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2/
ID AttachType AttachFlags Name
131 egress override pkt_cntr_4
And with 'effective' option it shows all effective programs for cg2:
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2/ effective
ID AttachType AttachFlags Name
131 egress override pkt_cntr_4
129 egress override pkt_cntr_1
130 egress override pkt_cntr_2
Signed-off-by: Takshak Chahande <ctakshak@fb.com>
---
Changelog:
v2:
- Removed global '-e|--effective' query flag
- Added 'effective' option after cgroup path argument as proposed by Jakub
.../bpftool/Documentation/bpftool-cgroup.rst | 17 +++--
tools/bpf/bpftool/bash-completion/bpftool | 15 +++--
tools/bpf/bpftool/cgroup.c | 66 ++++++++++++-------
3 files changed, 66 insertions(+), 32 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index 585f270c2d25..8c12e8bb5dd4 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -20,8 +20,8 @@ SYNOPSIS
CGROUP COMMANDS
===============
-| **bpftool** **cgroup { show | list }** *CGROUP*
-| **bpftool** **cgroup tree** [*CGROUP_ROOT*]
+| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
+| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
| **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
| **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
| **bpftool** **cgroup help**
@@ -35,13 +35,18 @@ CGROUP COMMANDS
DESCRIPTION
===========
- **bpftool cgroup { show | list }** *CGROUP*
+ **bpftool cgroup { show | list }** *CGROUP* [**effective**]
List all programs attached to the cgroup *CGROUP*.
Output will start with program ID followed by attach type,
attach flags and program name.
- **bpftool cgroup tree** [*CGROUP_ROOT*]
+ If **effective** is specified, then it retrieves all the
+ effective programs that will be executed for events within
+ a cgroup. This includes inherited along with attached ones
+ to the cgroup.
+
+ **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
Iterate over all cgroups in *CGROUP_ROOT* and list all
attached programs. If *CGROUP_ROOT* is not specified,
bpftool uses cgroup v2 mountpoint.
@@ -50,6 +55,10 @@ DESCRIPTION
commands: it starts with absolute cgroup path, followed by
program ID, attach type, attach flags and program name.
+ With **effective** it retrieves all the effective programs
+ that will be executed for events at each cgroup level as
+ similar to show/list command output for a cgroup.
+
**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
Attach program *PROG* to the cgroup *CGROUP* with attach type
*ATTACH_TYPE* and optional *ATTACH_FLAGS*.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index ba37095e1f62..b9b76b812dda 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -679,12 +679,15 @@ _bpftool()
;;
cgroup)
case $command in
- show|list)
- _filedir
- return 0
- ;;
- tree)
- _filedir
+ show|list|tree)
+ case $cword in
+ 3)
+ _filedir
+ ;;
+ 4)
+ COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) )
+ ;;
+ esac
return 0
;;
attach|detach)
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
index 390b89a224f1..09040dbdc20a 100644
--- a/tools/bpf/bpftool/cgroup.c
+++ b/tools/bpf/bpftool/cgroup.c
@@ -29,6 +29,7 @@
" recvmsg4 | recvmsg6 | sysctl |\n" \
" getsockopt | setsockopt }"
+static unsigned int query_flags;
static const char * const attach_type_strings[] = {
[BPF_CGROUP_INET_INGRESS] = "ingress",
[BPF_CGROUP_INET_EGRESS] = "egress",
@@ -107,7 +108,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
__u32 prog_cnt = 0;
int ret;
- ret = bpf_prog_query(cgroup_fd, type, 0, NULL, NULL, &prog_cnt);
+ ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
+ &prog_cnt);
if (ret)
return -1;
@@ -125,8 +127,8 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
int ret;
prog_cnt = ARRAY_SIZE(prog_ids);
- ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids,
- &prog_cnt);
+ ret = bpf_prog_query(cgroup_fd, type, query_flags, &attach_flags,
+ prog_ids, &prog_cnt);
if (ret)
return ret;
@@ -158,20 +160,32 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
static int do_show(int argc, char **argv)
{
enum bpf_attach_type type;
+ char *cgroup_path;
int cgroup_fd;
int ret = -1;
- if (argc < 1) {
+ if (!REQ_ARGS(1)) {
p_err("too few parameters for cgroup show");
goto exit;
- } else if (argc > 1) {
- p_err("too many parameters for cgroup show");
- goto exit;
}
- cgroup_fd = open(argv[0], O_RDONLY);
+ query_flags = 0;
+ cgroup_path = GET_ARG();
+
+ if (argc) {
+ if (argc == 1 && is_prefix(*argv, "effective")) {
+ query_flags |= BPF_F_QUERY_EFFECTIVE;
+ NEXT_ARG();
+ } else {
+ p_err("invalid argument after cgroup path, "
+ "got: '%s'?, expect only effective", *argv);
+ goto exit;
+ }
+ }
+
+ cgroup_fd = open(cgroup_path, O_RDONLY);
if (cgroup_fd < 0) {
- p_err("can't open cgroup %s", argv[1]);
+ p_err("can't open cgroup %s", cgroup_path);
goto exit;
}
@@ -294,26 +308,34 @@ static char *find_cgroup_root(void)
static int do_show_tree(int argc, char **argv)
{
+ bool free_cg_root = false;
char *cgroup_root;
int ret;
- switch (argc) {
- case 0:
+ query_flags = 0;
+
+ if (!argc) {
cgroup_root = find_cgroup_root();
if (!cgroup_root) {
p_err("cgroup v2 isn't mounted");
return -1;
}
- break;
- case 1:
- cgroup_root = argv[0];
- break;
- default:
- p_err("too many parameters for cgroup tree");
- return -1;
+ free_cg_root = true;
+ } else {
+ cgroup_root = GET_ARG();
+
+ if (argc) {
+ if (argc == 1 && is_prefix(*argv, "effective")) {
+ query_flags |= BPF_F_QUERY_EFFECTIVE;
+ NEXT_ARG();
+ } else {
+ p_err("invalid argument after cgroup path, "
+ "got: '%s'? expect only effective", *argv);
+ return -1;
+ }
+ }
}
-
if (json_output)
jsonw_start_array(json_wtr);
else
@@ -338,7 +360,7 @@ static int do_show_tree(int argc, char **argv)
if (json_output)
jsonw_end_array(json_wtr);
- if (argc == 0)
+ if (free_cg_root)
free(cgroup_root);
return ret;
@@ -459,8 +481,8 @@ static int do_help(int argc, char **argv)
}
fprintf(stderr,
- "Usage: %s %s { show | list } CGROUP\n"
- " %s %s tree [CGROUP_ROOT]\n"
+ "Usage: %s %s { show | list } CGROUP [effective]\n"
+ " %s %s tree [CGROUP_ROOT] [effective]\n"
" %s %s attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]\n"
" %s %s detach CGROUP ATTACH_TYPE PROG\n"
" %s %s help\n"
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the port parameters.
From: Andrew Lunn @ 2019-07-02 16:48 UTC (permalink / raw)
To: Parav Pandit
Cc: Sudarsana Reddy Kalluru, davem@davemloft.net,
netdev@vger.kernel.org, mkalderon@marvell.com, aelior@marvell.com,
jiri@resnulli.us
In-Reply-To: <AM0PR05MB4866D7B26F48AF0BED9055EED1F80@AM0PR05MB4866.eurprd05.prod.outlook.com>
> A vendor driver calling these APIs is needed at minimum.
Not a vendor driver, but a mainline driver.
But yes, a new API should not be added without at least one user.
Andrew
^ permalink raw reply
* Re: [PATCH 7/7] kbuild: compile-test kernel headers to ensure they are self-contained
From: Joel Fernandes @ 2019-07-02 16:47 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, Sam Ravnborg, Tony Luck, John Fastabend,
Jakub Kicinski, Daniel Borkmann, xdp-newbies, Anton Vorontsov,
Matthias Brugger, Song Liu, Yonghong Song, Michal Marek,
Jesper Dangaard Brouer, Martin KaFai Lau, linux-mediatek,
linux-arm-kernel, Colin Cross, David S. Miller, Kees Cook,
Alexei Starovoitov, netdev, linux-kernel, bpf
In-Reply-To: <20190701005845.12475-8-yamada.masahiro@socionext.com>
On Mon, Jul 01, 2019 at 09:58:45AM +0900, Masahiro Yamada wrote:
> The headers in include/ are globally used in the kernel source tree
> to provide common APIs. They are included from external modules, too.
>
> It will be useful to make as many headers self-contained as possible
> so that we do not have to rely on a specific include order.
>
> There are more than 4000 headers in include/. In my rough analysis,
> 70% of them are already self-contained. With efforts, most of them
> can be self-contained.
>
> For now, we must exclude more than 1000 headers just because they
> cannot be compiled as standalone units. I added them to header-test-.
> The blacklist was mostly generated by a script, so the reason of the
> breakage should be checked later.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Tested-by: Jani Nikula <jani.nikula@intel.com>
> ---
>
> Changes in v4:
> - Fix vmlinux build error
> - Exclude more headers for sparc
>
> Changes in v3:
> - Exclude more headers
> (Tested for allnoconfig + CONFIG_HEADER_TEST=y)
>
> Changes in v2:
> - Add everything to test coverage, and exclude broken ones
> - Rename 'Makefile' to 'Kbuild'
> - Add CONFIG_KERNEL_HEADER_TEST option
>
> Makefile | 1 +
> include/Kbuild | 1253 ++++++++++++++++++++++++++++++++++++++++++++++++
> init/Kconfig | 11 +
> 3 files changed, 1265 insertions(+)
> create mode 100644 include/Kbuild
[snip
> diff --git a/init/Kconfig b/init/Kconfig
> index 74192de8ada6..e2e99544da8d 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -108,6 +108,17 @@ config HEADER_TEST
> If you are a developer or tester and want to ensure the requested
> headers are self-contained, say Y here. Otherwise, choose N.
>
> +config KERNEL_HEADER_TEST
> + bool "Compile test kernel headers"
> + depends on HEADER_TEST
> + help
> + Headers in include/ are used to build external moduls.
Nit:
modules.
Otherwise lgtm, thanks for the cc.
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
^ permalink raw reply
* Re: [PATCH 2/3 bpf-next] i40e: Support zero-copy XDP_TX on the RX path for AF_XDP sockets.
From: Jonathan Lemon @ 2019-07-02 16:44 UTC (permalink / raw)
To: Magnus Karlsson
Cc: Network Development, Björn Töpel, Karlsson, Magnus,
Jakub Kicinski, jeffrey.t.kirsher, kernel-team
In-Reply-To: <CAJ8uoz0EL9gx87JmhjBmYscx-J2UCYK73OV73T3eohOEp0BEUw@mail.gmail.com>
On 1 Jul 2019, at 4:04, Magnus Karlsson wrote:
> On Sat, Jun 29, 2019 at 12:18 AM Jonathan Lemon
> <jonathan.lemon@gmail.com> wrote:
>>
>> When the XDP program attached to a zero-copy AF_XDP socket returns
>> XDP_TX,
>> queue the umem frame on the XDP TX ring. Space on the recycle stack
>> is
>> pre-allocated when the xsk is created. (taken from tx_ring, since
>> the
>> xdp ring is not initialized yet)
>>
>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>> ---
>> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 1 +
>> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 54
>> +++++++++++++++++++--
>> 2 files changed, 51 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> index 100e92d2982f..3e7954277737 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> @@ -274,6 +274,7 @@ static inline unsigned int
>> i40e_txd_use_count(unsigned int size)
>> #define I40E_TX_FLAGS_TSYN BIT(8)
>> #define I40E_TX_FLAGS_FD_SB BIT(9)
>> #define I40E_TX_FLAGS_UDP_TUNNEL BIT(10)
>> +#define I40E_TX_FLAGS_ZC_FRAME BIT(11)
>> #define I40E_TX_FLAGS_VLAN_MASK 0xffff0000
>> #define I40E_TX_FLAGS_VLAN_PRIO_MASK 0xe0000000
>> #define I40E_TX_FLAGS_VLAN_PRIO_SHIFT 29
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> index ce8650d06962..020f9859215d 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> @@ -91,7 +91,8 @@ static int i40e_xsk_umem_enable(struct i40e_vsi
>> *vsi, struct xdp_umem *umem,
>> qid >= netdev->real_num_tx_queues)
>> return -EINVAL;
>>
>> - if (!xsk_umem_recycle_alloc(umem, vsi->rx_rings[0]->count))
>> + if (!xsk_umem_recycle_alloc(umem, vsi->rx_rings[0]->count +
>> + vsi->tx_rings[0]->count))
>> return -ENOMEM;
>>
>> err = i40e_xsk_umem_dma_map(vsi, umem);
>> @@ -175,6 +176,48 @@ int i40e_xsk_umem_setup(struct i40e_vsi *vsi,
>> struct xdp_umem *umem,
>> i40e_xsk_umem_disable(vsi, qid);
>> }
>>
>> +static int i40e_xmit_rcvd_zc(struct i40e_ring *rx_ring, struct
>> xdp_buff *xdp)
>
> This function looks very much like i40e_xmit_xdp_ring(). How can we
> refactor them to make them share more code and not lose performance at
> the same time? This comment is also valid for the ixgbe driver patch
> that follows.
The next patch will split these into a small preamble setup and then
call a common send function.
--
Jonathan
>
> Thanks: Magnus
>
>> +{
>> + struct i40e_ring *xdp_ring;
>> + struct i40e_tx_desc *tx_desc;
>> + struct i40e_tx_buffer *tx_bi;
>> + struct xdp_frame *xdpf;
>> + dma_addr_t dma;
>> +
>> + xdp_ring = rx_ring->vsi->xdp_rings[rx_ring->queue_index];
>> +
>> + if (!unlikely(I40E_DESC_UNUSED(xdp_ring))) {
>> + xdp_ring->tx_stats.tx_busy++;
>> + return I40E_XDP_CONSUMED;
>> + }
>> + xdpf = convert_to_xdp_frame_keep_zc(xdp);
>> + if (unlikely(!xdpf))
>> + return I40E_XDP_CONSUMED;
>> + xdpf->handle = xdp->handle;
>> +
>> + dma = xdp_umem_get_dma(rx_ring->xsk_umem, xdp->handle);
>> + tx_bi = &xdp_ring->tx_bi[xdp_ring->next_to_use];
>> + tx_bi->bytecount = xdpf->len;
>> + tx_bi->gso_segs = 1;
>> + tx_bi->xdpf = xdpf;
>> + tx_bi->tx_flags = I40E_TX_FLAGS_ZC_FRAME;
>> +
>> + tx_desc = I40E_TX_DESC(xdp_ring, xdp_ring->next_to_use);
>> + tx_desc->buffer_addr = cpu_to_le64(dma);
>> + tx_desc->cmd_type_offset_bsz =
>> build_ctob(I40E_TX_DESC_CMD_ICRC |
>> +
>> I40E_TX_DESC_CMD_EOP,
>> + 0, xdpf->len, 0);
>> + smp_wmb();
>> +
>> + xdp_ring->next_to_use++;
>> + if (xdp_ring->next_to_use == xdp_ring->count)
>> + xdp_ring->next_to_use = 0;
>> +
>> + tx_bi->next_to_watch = tx_desc;
>> +
>> + return I40E_XDP_TX;
>> +}
>> +
>> /**
>> * i40e_run_xdp_zc - Executes an XDP program on an xdp_buff
>> * @rx_ring: Rx ring
>> @@ -187,7 +230,6 @@ int i40e_xsk_umem_setup(struct i40e_vsi *vsi,
>> struct xdp_umem *umem,
>> static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct
>> xdp_buff *xdp)
>> {
>> int err, result = I40E_XDP_PASS;
>> - struct i40e_ring *xdp_ring;
>> struct bpf_prog *xdp_prog;
>> u32 act;
>>
>> @@ -202,8 +244,7 @@ static int i40e_run_xdp_zc(struct i40e_ring
>> *rx_ring, struct xdp_buff *xdp)
>> case XDP_PASS:
>> break;
>> case XDP_TX:
>> - xdp_ring =
>> rx_ring->vsi->xdp_rings[rx_ring->queue_index];
>> - result = i40e_xmit_xdp_tx_ring(xdp, xdp_ring);
>> + result = i40e_xmit_rcvd_zc(rx_ring, xdp);
>> break;
>> case XDP_REDIRECT:
>> err = xdp_do_redirect(rx_ring->netdev, xdp,
>> xdp_prog);
>> @@ -628,6 +669,11 @@ static bool i40e_xmit_zc(struct i40e_ring
>> *xdp_ring, unsigned int budget)
>> static void i40e_clean_xdp_tx_buffer(struct i40e_ring *tx_ring,
>> struct i40e_tx_buffer *tx_bi)
>> {
>> + if (tx_bi->tx_flags & I40E_TX_FLAGS_ZC_FRAME) {
>> + xsk_umem_recycle_addr(tx_ring->xsk_umem,
>> tx_bi->xdpf->handle);
>> + tx_bi->tx_flags = 0;
>> + return;
>> + }
>> xdp_return_frame(tx_bi->xdpf);
>> dma_unmap_single(tx_ring->dev,
>> dma_unmap_addr(tx_bi, dma),
>> --
>> 2.17.1
>>
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Y Song @ 2019-07-02 16:42 UTC (permalink / raw)
To: Ilya Leoshkevich; +Cc: bpf, netdev
In-Reply-To: <20190702153908.41562-1-iii@linux.ibm.com>
On Tue, Jul 2, 2019 at 8:40 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Use PT_REGS_RC(ctx) instead of ctx->rax, which is not present on s390.
>
> Pass -D__TARGET_ARCH_$(ARCH) to selftests in order to choose a proper
> PT_REGS_RC variant.
>
> Fix s930 -> s390 typo.
>
> On s390, provide the forward declaration of struct pt_regs and cast it
> to user_pt_regs in PT_REGS_* macros. This is necessary, because instead
> of the full struct pt_regs, s390 exposes only its first field
> user_pt_regs to userspace, and bpf_helpers.h is used with both userspace
> (in selftests) and kernel (in samples) headers.
>
> On x86, provide userspace versions of PT_REGS_* macros. Unlike s390, x86
> provides struct pt_regs to both userspace and kernel, however, with
> different field names.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> tools/testing/selftests/bpf/Makefile | 4 +-
> tools/testing/selftests/bpf/bpf_helpers.h | 46 +++++++++++++++--------
> tools/testing/selftests/bpf/progs/loop1.c | 2 +-
> tools/testing/selftests/bpf/progs/loop2.c | 2 +-
> tools/testing/selftests/bpf/progs/loop3.c | 2 +-
> 5 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index d60fee59fbd1..599b320bef65 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> include ../../../../scripts/Kbuild.include
> +include ../../../scripts/Makefile.arch
>
> LIBDIR := ../../../lib
> BPFDIR := $(LIBDIR)/bpf
> @@ -138,7 +139,8 @@ CLANG_SYS_INCLUDES := $(shell $(CLANG) -v -E - </dev/null 2>&1 \
>
> CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
> $(CLANG_SYS_INCLUDES) \
> - -Wno-compare-distinct-pointer-types
> + -Wno-compare-distinct-pointer-types \
> + -D__TARGET_ARCH_$(ARCH)
>
> $(OUTPUT)/test_l4lb_noinline.o: CLANG_FLAGS += -fno-inline
> $(OUTPUT)/test_xdp_noinline.o: CLANG_FLAGS += -fno-inline
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index 1a5b1accf091..faf86d83301a 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -312,8 +312,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #if defined(__TARGET_ARCH_x86)
> #define bpf_target_x86
> #define bpf_target_defined
> -#elif defined(__TARGET_ARCH_s930x)
> - #define bpf_target_s930x
> +#elif defined(__TARGET_ARCH_s390)
> + #define bpf_target_s390
> #define bpf_target_defined
> #elif defined(__TARGET_ARCH_arm)
> #define bpf_target_arm
> @@ -338,8 +338,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #ifndef bpf_target_defined
> #if defined(__x86_64__)
> #define bpf_target_x86
> -#elif defined(__s390x__)
> - #define bpf_target_s930x
I see in some other places (e.g., bcc) where
macro __s390x__ is also used to indicate a s390 architecture.
Could you explain the difference between __s390__ and
__s390x__?
> +#elif defined(__s390__)
> + #define bpf_target_s390
> #elif defined(__arm__)
> #define bpf_target_arm
> #elif defined(__aarch64__)
> @@ -355,6 +355,7 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
>
> #if defined(bpf_target_x86)
>
> +#ifdef __KERNEL__
In samples/bpf/, __KERNEL__ is defined at clang options and
in selftests/bpf/, the __KERNEL__ is not defined.
I checked x86 pt_regs definition with and without __KERNEL__.
They are identical except some register name difference.
I am wondering whether we can unify into all without
__KERNEL__. Is __KERNEL__ really needed?
> #define PT_REGS_PARM1(x) ((x)->di)
> #define PT_REGS_PARM2(x) ((x)->si)
> #define PT_REGS_PARM3(x) ((x)->dx)
> @@ -365,19 +366,34 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #define PT_REGS_RC(x) ((x)->ax)
> #define PT_REGS_SP(x) ((x)->sp)
> #define PT_REGS_IP(x) ((x)->ip)
> +#else
> +#define PT_REGS_PARM1(x) ((x)->rdi)
> +#define PT_REGS_PARM2(x) ((x)->rsi)
> +#define PT_REGS_PARM3(x) ((x)->rdx)
> +#define PT_REGS_PARM4(x) ((x)->rcx)
> +#define PT_REGS_PARM5(x) ((x)->r8)
> +#define PT_REGS_RET(x) ((x)->rsp)
> +#define PT_REGS_FP(x) ((x)->rbp)
> +#define PT_REGS_RC(x) ((x)->rax)
> +#define PT_REGS_SP(x) ((x)->rsp)
> +#define PT_REGS_IP(x) ((x)->rip)
> +#endif
>
> -#elif defined(bpf_target_s390x)
> +#elif defined(bpf_target_s390)
>
> -#define PT_REGS_PARM1(x) ((x)->gprs[2])
> -#define PT_REGS_PARM2(x) ((x)->gprs[3])
> -#define PT_REGS_PARM3(x) ((x)->gprs[4])
> -#define PT_REGS_PARM4(x) ((x)->gprs[5])
> -#define PT_REGS_PARM5(x) ((x)->gprs[6])
> -#define PT_REGS_RET(x) ((x)->gprs[14])
> -#define PT_REGS_FP(x) ((x)->gprs[11]) /* Works only with CONFIG_FRAME_POINTER */
> -#define PT_REGS_RC(x) ((x)->gprs[2])
> -#define PT_REGS_SP(x) ((x)->gprs[15])
> -#define PT_REGS_IP(x) ((x)->psw.addr)
> +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
> +struct pt_regs;
> +#define PT_REGS_PARM1(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> +#define PT_REGS_PARM2(x) (((const volatile user_pt_regs *)(x))->gprs[3])
> +#define PT_REGS_PARM3(x) (((const volatile user_pt_regs *)(x))->gprs[4])
> +#define PT_REGS_PARM4(x) (((const volatile user_pt_regs *)(x))->gprs[5])
> +#define PT_REGS_PARM5(x) (((const volatile user_pt_regs *)(x))->gprs[6])
> +#define PT_REGS_RET(x) (((const volatile user_pt_regs *)(x))->gprs[14])
> +/* Works only with CONFIG_FRAME_POINTER */
> +#define PT_REGS_FP(x) (((const volatile user_pt_regs *)(x))->gprs[11])
> +#define PT_REGS_RC(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> +#define PT_REGS_SP(x) (((const volatile user_pt_regs *)(x))->gprs[15])
> +#define PT_REGS_IP(x) (((const volatile user_pt_regs *)(x))->psw.addr)
Is user_pt_regs a recent change or has been there for quite some time?
I am asking since bcc did not use user_pt_regs yet.
>
> #elif defined(bpf_target_arm)
>
> diff --git a/tools/testing/selftests/bpf/progs/loop1.c b/tools/testing/selftests/bpf/progs/loop1.c
> index dea395af9ea9..7cdb7f878310 100644
> --- a/tools/testing/selftests/bpf/progs/loop1.c
> +++ b/tools/testing/selftests/bpf/progs/loop1.c
> @@ -18,7 +18,7 @@ int nested_loops(volatile struct pt_regs* ctx)
> for (j = 0; j < 300; j++)
> for (i = 0; i < j; i++) {
> if (j & 1)
> - m = ctx->rax;
> + m = PT_REGS_RC(ctx);
> else
> m = j;
> sum += i * m;
> diff --git a/tools/testing/selftests/bpf/progs/loop2.c b/tools/testing/selftests/bpf/progs/loop2.c
> index 0637bd8e8bcf..9b2f808a2863 100644
> --- a/tools/testing/selftests/bpf/progs/loop2.c
> +++ b/tools/testing/selftests/bpf/progs/loop2.c
> @@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
> int i = 0;
>
> while (true) {
> - if (ctx->rax & 1)
> + if (PT_REGS_RC(ctx) & 1)
> i += 3;
> else
> i += 7;
> diff --git a/tools/testing/selftests/bpf/progs/loop3.c b/tools/testing/selftests/bpf/progs/loop3.c
> index 30a0f6cba080..d727657d51e2 100644
> --- a/tools/testing/selftests/bpf/progs/loop3.c
> +++ b/tools/testing/selftests/bpf/progs/loop3.c
> @@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
> __u64 i = 0, sum = 0;
> do {
> i++;
> - sum += ctx->rax;
> + sum += PT_REGS_RC(ctx);
> } while (i < 0x100000000ULL);
> return sum;
> }
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH v2 bpf-next] libbpf: fix GCC8 warning for strncpy
From: Magnus Karlsson @ 2019-07-02 16:38 UTC (permalink / raw)
To: Y Song
Cc: Andrii Nakryiko, Andrii Nakryiko, bpf, netdev, Alexei Starovoitov,
Daniel Borkmann, Magnus Karlsson
In-Reply-To: <CAH3MdRWeH=Ko_mAvWk2mUaMK50iNHLbZkHKK=dVTzuwihZeRuA@mail.gmail.com>
On Tue, Jul 2, 2019 at 6:11 PM Y Song <ys114321@gmail.com> wrote:
>
> On Tue, Jul 2, 2019 at 8:17 AM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > GCC8 started emitting warning about using strncpy with number of bytes
> > exactly equal destination size, which is generally unsafe, as can lead
> > to non-zero terminated string being copied. Use IFNAMSIZ - 1 as number
> > of bytes to ensure name is always zero-terminated.
> >
> > Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Acked-by: Yonghong Song <yhs@fb.com>
Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
>
> > ---
> > tools/lib/bpf/xsk.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > index bf15a80a37c2..b33740221b7e 100644
> > --- a/tools/lib/bpf/xsk.c
> > +++ b/tools/lib/bpf/xsk.c
> > @@ -327,7 +327,8 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
> >
> > channels.cmd = ETHTOOL_GCHANNELS;
> > ifr.ifr_data = (void *)&channels;
> > - strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ);
> > + strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ - 1);
> > + ifr.ifr_name[IFNAMSIZ - 1] = '\0';
> > err = ioctl(fd, SIOCETHTOOL, &ifr);
> > if (err && errno != EOPNOTSUPP) {
> > ret = -errno;
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [RFC PATCH v2 2/2] Documentation: net: dsa: b53: Describe b53 configuration
From: Benedikt Spranger @ 2019-07-02 16:28 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, netdev, Sebastian Andrzej Siewior,
Kurt Kanzenbach, Vivien Didelot
In-Reply-To: <20190701173550.GH30468@lunn.ch>
Am Mon, 1 Jul 2019 19:35:50 +0200
schrieb Andrew Lunn <andrew@lunn.ch>:
> > +Configuration without tagging support
> > +-------------------------------------
>
> How does this differ to the text you just added in the previous patch?
The b53 has some implementation specific detail:
The b53 DSA driver tags the CPU port in all VLANs, since otherwise any
PVID untagged VLAN programming would basically change the CPU port's
default PVID and make it untagged, undesirable.
This need some attention while configuring. Therefore the configuration
is slightly different to the generic one:
The following extra commands are needed on b53 single port and gateway
configuration:
bridge vlan del dev lan1 vid 1
bridge vlan del dev lan2 vid 1
On bridge config the following commands are not needed:
bridge vlan add dev lan1 vid 1 pvid untagged
bridge vlan add dev lan2 vid 1 pvid untagged
bridge vlan add dev lan3 vid 1 pvid untagged
> Do we need both?
I like full configuration examples. But it can also be done by
describing the changes. I would prefer both - description and full
script:
...
Configuration without tagging support
-------------------------------------
Older models (5325, 5365) support a different tag format that is not
supported yet. 539x and 531x5 require managed mode and some special
handling, which is also not yet supported. The tagging support is
disabled in these cases and the switch need a different configuration.
The configuration slightly differ from
the :ref:`dsa-vlan-configuration`.
+ The b53 tags the CPU port in all VLANs, since otherwise any PVID
+ untagged VLAN programming would basically change the CPU port's
+ default PVID and make it untagged, undesirable.
+ In difference to the configuration described
+ in :ref:`dsa-vlan-configuration` the default VLAN 1 has to be removed
+ from the slave interface configuration in single port and gateway
+ configuration, while there is no need to add an extra VLAN
+ configuration in the bridge showcase.
single port
~~~~~~~~~~~
...
Regards
Bene Spranger
^ permalink raw reply
* Re: [PATCH net-next v6 01/15] rtnetlink: provide permanent hardware address in RTM_NEWLINK
From: Michal Kubecek @ 2019-07-02 16:35 UTC (permalink / raw)
To: netdev
Cc: Stephen Hemminger, David Miller, Jakub Kicinski, Jiri Pirko,
Andrew Lunn, Florian Fainelli, John Linville, Johannes Berg,
linux-kernel
In-Reply-To: <20190702075500.1b9845e1@hermes.lan>
On Tue, Jul 02, 2019 at 07:55:00AM -0700, Stephen Hemminger wrote:
> On Tue, 2 Jul 2019 13:49:44 +0200 (CEST)
> Michal Kubecek <mkubecek@suse.cz> wrote:
>
> > Permanent hardware address of a network device was traditionally provided
> > via ethtool ioctl interface but as Jiri Pirko pointed out in a review of
> > ethtool netlink interface, rtnetlink is much more suitable for it so let's
> > add it to the RTM_NEWLINK message.
> >
> > Add IFLA_PERM_ADDRESS attribute to RTM_NEWLINK messages unless the
> > permanent address is all zeros (i.e. device driver did not fill it). As
> > permanent address is not modifiable, reject userspace requests containing
> > IFLA_PERM_ADDRESS attribute.
> >
> > Note: we already provide permanent hardware address for bond slaves;
> > unfortunately we cannot drop that attribute for backward compatibility
> > reasons.
> >
> > v5 -> v6: only add the attribute if permanent address is not zero
> >
> > Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>
> Do you want to make an iproute patch to display this?
Yes, I'm going to submit it once this patch gets into net-next.
Michal
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply
* Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink interface
From: Michal Kubecek @ 2019-07-02 16:34 UTC (permalink / raw)
To: netdev
Cc: Jiri Pirko, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190702130515.GO2250@nanopsycho>
On Tue, Jul 02, 2019 at 03:05:15PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:50:04PM CEST, mkubecek@suse.cz wrote:
> >
> >+/* request header */
> >+
> >+/* use compact bitsets in reply */
> >+#define ETHTOOL_RF_COMPACT (1 << 0)
>
> "COMPACT_BITSETS"?
>
> >+/* provide optional reply for SET or ACT requests */
> >+#define ETHTOOL_RF_REPLY (1 << 1)
>
> "OPTIONAL_REPLY"?
OK
> >+ ret = nla_parse_nested(tb, ETHTOOL_A_HEADER_MAX, nest,
> >+ policy ?: dflt_header_policy, extack);
> >+ if (ret < 0)
>
> if (ret)
>
> Same remark goes to the rest of the code (also the rest of the patches),
> in case called function cannot return positive values.
The "if (ret < 0)" idiom for "on error do ..." is so ubiquitous through
the whole kernel that I don't think it's worth it to carefully check
which function can return a positive value and which cannot and risk
that one day I overlook that some function. And yet another question is
what exactly "cannot return" means: is it whenever the function does not
return a positive value or only if it's explicitly documented not to?
Looking at existing networking code, e.g. net/netfilter (except ipvs),
net/sched or net/core/rtnetlink.c are using "if (ret < 0)" rather
uniformly. And (as you objected to the check of genl_register_family()
previous patch) even genetlink itself has
err = genl_register_family(&genl_ctrl);
if (err < 0)
goto problem;
in genl_init().
>
>
> >+ return ret;
> >+ devname_attr = tb[ETHTOOL_A_HEADER_DEV_NAME];
> >+
> >+ if (tb[ETHTOOL_A_HEADER_DEV_INDEX]) {
> >+ u32 ifindex = nla_get_u32(tb[ETHTOOL_A_HEADER_DEV_INDEX]);
> >+
> >+ dev = dev_get_by_index(net, ifindex);
> >+ if (!dev) {
> >+ NL_SET_ERR_MSG_ATTR(extack,
> >+ tb[ETHTOOL_A_HEADER_DEV_INDEX],
> >+ "no device matches ifindex");
> >+ return -ENODEV;
> >+ }
> >+ /* if both ifindex and ifname are passed, they must match */
> >+ if (devname_attr &&
> >+ strncmp(dev->name, nla_data(devname_attr), IFNAMSIZ)) {
> >+ dev_put(dev);
> >+ NL_SET_ERR_MSG_ATTR(extack, nest,
> >+ "ifindex and name do not match");
> >+ return -ENODEV;
> >+ }
> >+ } else if (devname_attr) {
> >+ dev = dev_get_by_name(net, nla_data(devname_attr));
> >+ if (!dev) {
> >+ NL_SET_ERR_MSG_ATTR(extack, devname_attr,
> >+ "no device matches name");
> >+ return -ENODEV;
> >+ }
> >+ } else if (require_dev) {
> >+ NL_SET_ERR_MSG_ATTR(extack, nest,
> >+ "neither ifindex nor name specified");
> >+ return -EINVAL;
> >+ }
> >+
> >+ if (dev && !netif_device_present(dev)) {
> >+ dev_put(dev);
> >+ NL_SET_ERR_MSG(extack, "device not present");
> >+ return -ENODEV;
> >+ }
> >+
> >+ req_info->dev = dev;
> >+ ethnl_update_u32(&req_info->req_mask, tb[ETHTOOL_A_HEADER_INFOMASK]);
> >+ ethnl_update_u32(&req_info->global_flags, tb[ETHTOOL_A_HEADER_GFLAGS]);
> >+ ethnl_update_u32(&req_info->req_flags, tb[ETHTOOL_A_HEADER_RFLAGS]);
>
> Just:
> req_info->req_mask = nla_get_u32(tb[ETHTOOL_A_HEADER_INFOMASK];
> ...
>
> Not sure what ethnl_update_u32() is good for, but it is not needed here.
That would result in null pointer dereference if the attribute is
missing. So you would need at least
if (tb[ETHTOOL_A_HEADER_INFOMASK])
req_info->req_mask = nla_get_u32(tb[ETHTOOL_A_HEADER_INFOMASK]);
if (tb[ETHTOOL_A_HEADER_GFLAGS])
req_info->global_flags =
nla_get_u32(tb[ETHTOOL_A_HEADER_GFLAGS]);
if (tb[ETHTOOL_A_HEADER_RFLAGS])
req_info->req_flags = nla_get_u32(tb[ETHTOOL_A_HEADER_RFLAGS]);
I don't think it looks better.
> >+
> >+ return 0;
> >+}
> >+
> >+/**
> >+ * ethnl_fill_reply_header() - Put standard header into a reply message
> >+ * @skb: skb with the message
> >+ * @dev: network device to describe in header
> >+ * @attrtype: attribute type to use for the nest
> >+ *
> >+ * Create a nested attribute with attributes describing given network device.
> >+ * Clean up on error.
>
> Cleanup is obvious, no need to mention it in API docs.
OK
> >+ *
> >+ * Return: 0 on success, error value (-EMSGSIZE only) on error
> >+ */
> >+int ethnl_fill_reply_header(struct sk_buff *skb, struct net_device *dev,
> >+ u16 attrtype)
> >+{
> >+ struct nlattr *nest;
> >+
> >+ if (!dev)
> >+ return 0;
> >+ nest = nla_nest_start(skb, attrtype);
> >+ if (!nest)
> >+ return -EMSGSIZE;
> >+
> >+ if (nla_put_u32(skb, ETHTOOL_A_HEADER_DEV_INDEX, (u32)dev->ifindex) ||
> >+ nla_put_string(skb, ETHTOOL_A_HEADER_DEV_NAME, dev->name))
> >+ goto nla_put_failure;
> >+ /* If more attributes are put into reply header, ethnl_header_size()
> >+ * must be updated to account for them.
> >+ */
> >+
> >+ nla_nest_end(skb, nest);
> >+ return 0;
> >+
> >+nla_put_failure:
> >+ nla_nest_cancel(skb, nest);
> >+ return -EMSGSIZE;
> >+}
> >+
> >+/**
> >+ * ethnl_reply_init() - Create skb for a reply and fill device identification
> >+ * @payload: payload length (without netlink and genetlink header)
> >+ * @dev: device the reply is about (may be null)
> >+ * @cmd: ETHTOOL_MSG_* message type for reply
> >+ * @info: genetlink info of the received packet we respond to
> >+ * @ehdrp: place to store payload pointer returned by genlmsg_new()
> >+ *
> >+ * Return: pointer to allocated skb on success, NULL on error
> >+ */
> >+struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
> >+ u16 hdr_attrtype, struct genl_info *info,
> >+ void **ehdrp)
> >+{
> >+ struct sk_buff *skb;
> >+
> >+ skb = genlmsg_new(payload, GFP_KERNEL);
> >+ if (!skb)
> >+ goto err;
> >+ *ehdrp = genlmsg_put_reply(skb, info, ðtool_genl_family, 0, cmd);
> >+ if (!*ehdrp)
> >+ goto err_free;
> >+
> >+ if (dev) {
> >+ int ret;
> >+
> >+ ret = ethnl_fill_reply_header(skb, dev, hdr_attrtype);
> >+ if (ret < 0)
> >+ goto err;
> >+ }
> >+ return skb;
> >+
> >+err_free:
> >+ nlmsg_free(skb);
> >+ if (info)
> >+ GENL_SET_ERR_MSG(info, "failed to setup reply message");
> >+err:
>
> Why also not fillup extack msg here?
Right, err label should be right below the nlmsg_free(skb), thanks. And
now I noticed another mistake: on ethnl_fill_reply_header() failure, we
should go to err_free, not err.
> >+static inline int ethnl_str_size(const char *s)
>
> If you really need this helper, put it into netlink code. There's nothing
> ethtool-specific about this.
OK, I'll look into it. I've been already thinking about some kind of
NLA_SIZEOF() macro as about 1/3 of all uses of nla_total_size() follow
the nla_total_size(sizeof(...)) pattern (and lot more should follow it
but are written like e.g. nla_total_size(4) instead). This is another
common pattern.
> >+/* The ethnl_update_* helpers set value pointed to by @dst to the value of
> >+ * netlink attribute @attr (if attr is not null). They return true if *dst
> >+ * value was changed, false if not.
> >+ */
> >+static inline bool ethnl_update_u32(u32 *dst, struct nlattr *attr)
>
> I'm still not sure I'm convinced about these "update helpers" :)
Just imagine what would e.g.
if (ethnl_update_u32(&data.rx_pending, tb[ETHTOOL_A_RING_RX_PENDING]))
mod = true;
if (ethnl_update_u32(&data.rx_mini_pending,
tb[ETHTOOL_A_RING_RX_MINI_PENDING]))
mod = true;
if (ethnl_update_u32(&data.rx_jumbo_pending,
tb[ETHTOOL_A_RING_RX_JUMBO_PENDING]))
mod = true;
if (ethnl_update_u32(&data.tx_pending, tb[ETHTOOL_A_RING_TX_PENDING]))
mod = true;
if (!mod)
return 0;
look like without them. And coalescing parameters would be much worse
(22 attributes / struct members).
> >+{
> >+ u32 val;
> >+
> >+ if (!attr)
> >+ return false;
> >+ val = nla_get_u32(attr);
> >+ if (*dst == val)
> >+ return false;
> >+
> >+ *dst = val;
> >+ return true;
> >+}
...
> >+static inline bool ethnl_update_binary(u8 *dst, unsigned int len,
>
> void *dst
OK.
> >+/**
> >+ * ethnl_is_privileged() - check if request has sufficient privileges
> >+ * @skb: skb with client request
> >+ *
> >+ * Checks if client request has CAP_NET_ADMIN in its netns. Unlike the flags
> >+ * in genl_ops, this allows finer access control, e.g. allowing or denying
> >+ * the request based on its contents or witholding only part of the data
> >+ * from unprivileged users.
> >+ *
> >+ * Return: true if request is privileged, false if not
> >+ */
> >+static inline bool ethnl_is_privileged(struct sk_buff *skb)
>
> I wonder why you need this helper. Genetlink uses
> ops->flags & GENL_ADMIN_PERM for this.
It's explained in the function description. Sometimes we need finer
control than by request message type. An example is the WoL password:
ETHTOOL_GWOL is privileged because of it but I believe there si no
reason why unprivileged user couldn't see enabled WoL modes, we can
simply omit the password for him. (Also, it allows to combine query for
WoL settings with other unprivileged settings.)
> >+/**
> >+ * ethnl_reply_header_size() - total size of reply header
> >+ *
> >+ * This is an upper estimate so that we do not need to hold RTNL lock longer
> >+ * than necessary (to prevent rename between size estimate and composing the
>
> I guess this description is not relevant anymore. I don't see why to
> hold rtnl mutex for this function...
You don't need it for this function, it's the other way around: unless
you hold RTNL lock for the whole time covering both checking needed
message size and filling the message - and we don't - the device could
be renamed in between. Thus if we returned size based on current device
name, it might not be sufficient at the time the header is filled.
That's why this function returns maximum possible size (which is
actually a constant).
Michal
> >+ * message). Accounts only for device ifindex and name as those are the only
> >+ * attributes ethnl_fill_reply_header() puts into the reply header.
> >+ */
> >+static inline unsigned int ethnl_reply_header_size(void)
> >+{
> >+ return nla_total_size(nla_total_size(sizeof(u32)) +
> >+ nla_total_size(IFNAMSIZ));
> >+}
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Jonathan Lemon @ 2019-07-02 16:33 UTC (permalink / raw)
To: Richardson, Bruce
Cc: Jakub Kicinski, Laatz, Kevin, netdev, ast, daniel, Topel, Bjorn,
Karlsson, Magnus, bpf, intel-wired-lan, Loftus, Ciara
In-Reply-To: <59AF69C657FD0841A61C55336867B5B07ED8B210@IRSMSX103.ger.corp.intel.com>
On 2 Jul 2019, at 2:27, Richardson, Bruce wrote:
>> -----Original Message-----
>> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
>> Sent: Monday, July 1, 2019 10:20 PM
>> To: Laatz, Kevin <kevin.laatz@intel.com>
>> Cc: Jonathan Lemon <jonathan.lemon@gmail.com>;
>> netdev@vger.kernel.org;
>> ast@kernel.org; daniel@iogearbox.net; Topel, Bjorn
>> <bjorn.topel@intel.com>; Karlsson, Magnus
>> <magnus.karlsson@intel.com>;
>> bpf@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Richardson,
>> Bruce
>> <bruce.richardson@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>
>> Subject: Re: [PATCH 00/11] XDP unaligned chunk placement support
>>
>> On Mon, 1 Jul 2019 15:44:29 +0100, Laatz, Kevin wrote:
>>> On 28/06/2019 21:29, Jonathan Lemon wrote:
>>>> On 28 Jun 2019, at 9:19, Laatz, Kevin wrote:
>>>>> On 27/06/2019 22:25, Jakub Kicinski wrote:
>>>>>> I think that's very limiting. What is the challenge in
>>>>>> providing
>>>>>> aligned addresses, exactly?
>>>>> The challenges are two-fold:
>>>>> 1) it prevents using arbitrary buffer sizes, which will be an
>>>>> issue
>>>>> supporting e.g. jumbo frames in future.
>>>>> 2) higher level user-space frameworks which may want to use
>>>>> AF_XDP,
>>>>> such as DPDK, do not currently support having buffers with 'fixed'
>>>>> alignment.
>>>>> The reason that DPDK uses arbitrary placement is that:
>>>>> - it would stop things working on certain NICs which
>>>>> need
>>>>> the actual writable space specified in units of 1k - therefore we
>>>>> need 2k
>>>>> + metadata space.
>>>>> - we place padding between buffers to avoid
>>>>> constantly
>>>>> hitting the same memory channels when accessing memory.
>>>>> - it allows the application to choose the actual
>>>>> buffer
>>>>> size it wants to use.
>>>>> We make use of the above to allow us to speed up processing
>>>>> significantly and also reduce the packet buffer memory size.
>>>>>
>>>>> Not having arbitrary buffer alignment also means an AF_XDP
>>>>> driver for DPDK cannot be a drop-in replacement for existing
>>>>> drivers in those frameworks. Even with a new capability to allow
>>>>> an
>>>>> arbitrary buffer alignment, existing apps will need to be modified
>>>>> to use that new capability.
>>>>
>>>> Since all buffers in the umem are the same chunk size, the original
>>>> buffer address can be recalculated with some multiply/shift math.
>>>> However, this is more expensive than just a mask operation.
>>>
>>> Yes, we can do this.
>>
>> That'd be best, can DPDK reasonably guarantee the slicing is uniform?
>> E.g. it's not desperate buffer pools with different bases?
>
> It's generally uniform, but handling the crossing of (huge)page
> boundaries
> complicates things a bit. Therefore I think the final option below
> is best as it avoids any such problems.
>
>>
>>> Another option we have is to add a socket option for querying the
>>> metadata length from the driver (assuming it doesn't vary per
>>> packet).
>>> We can use that information to get back to the original address
>>> using
>>> subtraction.
>>
>> Unfortunately the metadata depends on the packet and how much info
>> the
>> device was able to extract. So it's variable length.
>>
>>> Alternatively, we can change the Rx descriptor format to include the
>>> metadata length. We could do this in a couple of ways, for example,
>>> rather than returning the address as the start of the packet,
>>> instead
>>> return the buffer address that was passed in, and adding another
>>> 16-bit field to specify the start of packet offset with that buffer.
>>> If using another 16-bits of the descriptor space is not desirable,
>>> an
>>> alternative could be to limit umem sizes to e.g. 2^48 bits (256
>>> terabytes should be enough, right :-) ) and use the remaining 16
>>> bits
>>> of the address as a packet offset. Other variations on these
>>> approach
>>> are obviously possible too.
>>
>> Seems reasonable to me..
>
> I think this is probably the best solution, and also has the advantage
> that
> a buffer retains its base address the full way through the cycle of Rx
> and Tx.
I like this as well - it also has the advantage that drivers can keep
performing adjustments on the handle, which ends up just modifying the
offset.
--
Jonathan
^ permalink raw reply
* Re: [PATCH 2/3 bpf-next] i40e: Support zero-copy XDP_TX on the RX path for AF_XDP sockets.
From: Jonathan Lemon @ 2019-07-02 16:31 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: netdev, bjorn.topel, magnus.karlsson, jakub.kicinski,
jeffrey.t.kirsher, kernel-team
In-Reply-To: <f9c7b86c-91a6-9585-d1f1-f6f325794038@mellanox.com>
On 2 Jul 2019, at 0:07, Maxim Mikityanskiy wrote:
> On 2019-06-29 01:15, Jonathan Lemon wrote:
>> + xdpf = convert_to_xdp_frame_keep_zc(xdp);
>> + if (unlikely(!xdpf))
>> + return I40E_XDP_CONSUMED;
>> + xdpf->handle = xdp->handle;
>
> Shouldn't this line belong to convert_to_xdp_frame_keep_zc (and the
> previous patch)? It looks like it's code common for all drivers, and
> also patch 1 adds the handle field, but doesn't use it, which looks weird.
Good point. I'll move it into the function.
--
Jonathan
^ 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