* Re: [PATCH bpf-next] selftests: bpf: standardize to static __always_inline
From: Daniel Borkmann @ 2019-07-03 15:03 UTC (permalink / raw)
To: Jiri Benc, netdev, bpf; +Cc: Alexei Starovoitov, Song Liu, Y Song
In-Reply-To: <b62ef44131bbe3b905fc42990d16b667a65c820c.1562091849.git.jbenc@redhat.com>
On 07/02/2019 08:26 PM, Jiri Benc wrote:
> The progs for bpf selftests use several different notations to force
> function inlining. Standardize to what most of them use,
> static __always_inline.
>
> Suggested-by: Song Liu <liu.song.a23@gmail.com>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v4 bpf-next] bpf: Add support for fq's EDT to HBM
From: Daniel Borkmann @ 2019-07-03 15:03 UTC (permalink / raw)
To: brakmo, netdev
Cc: Alexei Starovoitov, Yonghong Song, Eric Dumazet, Kernel Team
In-Reply-To: <20190702220952.3929270-1-brakmo@fb.com>
On 07/03/2019 12:09 AM, brakmo wrote:
> Adds support for fq's Earliest Departure Time to HBM (Host Bandwidth
> Manager). Includes a new BPF program supporting EDT, and also updates
> corresponding programs.
>
> It will drop packets with an EDT of more than 500us in the future
> unless the packet belongs to a flow with less than 2 packets in flight.
> This is done so each flow has at least 2 packets in flight, so they
> will not starve, and also to help prevent delayed ACK timeouts.
>
> It will also work with ECN enabled traffic, where the packets will be
> CE marked if their EDT is more than 50us in the future.
>
> The table below shows some performance numbers. The flows are back to
> back RPCS. One server sending to another, either 2 or 4 flows.
> One flow is a 10KB RPC, the rest are 1MB RPCs. When there are more
> than one flow of a given RPC size, the numbers represent averages.
>
> The rate limit applies to all flows (they are in the same cgroup).
> Tests ending with "-edt" ran with the new BPF program supporting EDT.
> Tests ending with "-hbt" ran on top HBT qdisc with the specified rate
> (i.e. no HBM). The other tests ran with the HBM BPF program included
> in the HBM patch-set.
>
> EDT has limited value when using DCTCP, but it helps in many cases when
> using Cubic. It usually achieves larger link utilization and lower
> 99% latencies for the 1MB RPCs.
> HBM ends up queueing a lot of packets with its default parameter values,
> reducing the goodput of the 10KB RPCs and increasing their latency. Also,
> the RTTs seen by the flows are quite large.
>
> Aggr 10K 10K 10K 1MB 1MB 1MB
> Limit rate drops RTT rate P90 P99 rate P90 P99
> Test rate Flows Mbps % us Mbps us us Mbps ms ms
> -------- ---- ----- ---- ----- --- ---- ---- ---- ---- ---- ----
> cubic 1G 2 904 0.02 108 257 511 539 647 13.4 24.5
> cubic-edt 1G 2 982 0.01 156 239 656 967 743 14.0 17.2
> dctcp 1G 2 977 0.00 105 324 408 744 653 14.5 15.9
> dctcp-edt 1G 2 981 0.01 142 321 417 811 660 15.7 17.0
> cubic-htb 1G 2 919 0.00 1825 40 2822 4140 879 9.7 9.9
>
> cubic 200M 2 155 0.30 220 81 532 655 74 283 450
> cubic-edt 200M 2 188 0.02 222 87 1035 1095 101 84 85
> dctcp 200M 2 188 0.03 111 77 912 939 111 76 325
> dctcp-edt 200M 2 188 0.03 217 74 1416 1738 114 76 79
> cubic-htb 200M 2 188 0.00 5015 8 14ms 15ms 180 48 50
>
> cubic 1G 4 952 0.03 110 165 516 546 262 38 154
> cubic-edt 1G 4 973 0.01 190 111 1034 1314 287 65 79
> dctcp 1G 4 951 0.00 103 180 617 905 257 37 38
> dctcp-edt 1G 4 967 0.00 163 151 732 1126 272 43 55
> cubic-htb 1G 4 914 0.00 3249 13 7ms 8ms 300 29 34
>
> cubic 5G 4 4236 0.00 134 305 490 624 1310 10 17
> cubic-edt 5G 4 4865 0.00 156 306 425 759 1520 10 16
> dctcp 5G 4 4936 0.00 128 485 221 409 1484 7 9
> dctcp-edt 5G 4 4924 0.00 148 390 392 623 1508 11 26
>
> v1 -> v2: Incorporated Andrii's suggestions
> v2 -> v3: Incorporated Yonghong's suggestions
> v3 -> v4: Removed credit update that is not needed
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net-next] skbuff: increase verbosity when dumping skb data
From: Willem de Bruijn @ 2019-07-03 14:55 UTC (permalink / raw)
To: Yunsheng Lin
Cc: Network Development, David Miller, Cong Wang, Herbert Xu,
Eric Dumazet, Saeed Mahameed, Willem de Bruijn
In-Reply-To: <254abb52-e201-eb12-d6c2-6bd96e505871@huawei.com>
On Tue, Jul 2, 2019 at 11:28 PM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2019/7/3 3:39, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> >
> > skb_warn_bad_offload and netdev_rx_csum_fault trigger on hard to debug
> > issues. Dump more state and the header.
> >
> > Optionally dump the entire packet and linear segment. This is required
> > to debug checksum bugs that may include bytes past skb_tail_pointer().
> >
> > Both call sites call this function inside a net_ratelimit() block.
> > Limit full packet log further to a hard limit of can_dump_full (5).
> >
> > Based on an earlier patch by Cong Wang, see link below.
> >
> > Link: https://patchwork.ozlabs.org/patch/1000841/
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > ---
> > +void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt)
> > +{
> > + static atomic_t can_dump_full = ATOMIC_INIT(5);
> > + struct skb_shared_info *sh = skb_shinfo(skb);
> > + struct net_device *dev = skb->dev;
> > + struct sock *sk = skb->sk;
> > + struct sk_buff *list_skb;
> > + bool has_mac, has_trans;
> > + int headroom, tailroom;
> > + int i, len, seg_len;
> > +
> > + if (full_pkt)
> > + full_pkt = atomic_dec_if_positive(&can_dump_full) >= 0;
> > +
> > + if (full_pkt)
> > + len = skb->len;
>
> Minor question:
> Here we set the len to skb->len if full_pkt is true when skb_dump is
> called with frag_list skb and full_pkt being true below, which may
> cause some problem?
Good catch, thanks!
That recursive call to skb_dump on the frag_list below was not updated
from a previous revision that passed an explicit length.
> Maybe change the definition to:
> void skb_dump(const char *level, const struct sk_buff *skb, int len, bool full_pkt)
Indeed. It is less important when full_pkt, as then the entire
frag_list will be printed.
But if len is truncated, but somehow len != 0 when reaching the
frag_list, it might overshoot the limit. Will fix for v2.
> skb_dump(KERN_ERR, skb, skb->len, true);
>
> > + else
> > + len = min_t(int, skb->len, MAX_HEADER + 128);
> > +
> > + headroom = skb_headroom(skb);
> > + tailroom = skb_tailroom(skb);
> > +
> > + has_mac = skb_mac_header_was_set(skb);
> > + has_trans = skb_transport_header_was_set(skb);
> > +
> > + printk("%sskb len=%u headroom=%u headlen=%u tailroom=%u\n"
> > + "mac=(%d,%d) net=(%d,%d) trans=%d\n"
> > + "shinfo(txflags=%u nr_frags=%u gso(size=%hu type=%u segs=%hu))\n"
> > + "csum(0x%x ip_summed=%u complete_sw=%u valid=%u level=%u)\n"
> > + "hash(0x%x sw=%u l4=%u) proto=0x%04x pkttype=%u iif=%d\n",
> > + level, skb->len, headroom, skb_headlen(skb), tailroom,
> > + has_mac ? skb->mac_header : -1,
> > + has_mac ? skb_mac_header_len(skb) : -1,
> > + skb->network_header,
> > + has_trans ? skb_network_header_len(skb) : -1,
> > + has_trans ? skb->transport_header : -1,
> > + sh->tx_flags, sh->nr_frags,
> > + sh->gso_size, sh->gso_type, sh->gso_segs,
> > + skb->csum, skb->ip_summed, skb->csum_complete_sw,
> > + skb->csum_valid, skb->csum_level,
> > + skb->hash, skb->sw_hash, skb->l4_hash,
> > + ntohs(skb->protocol), skb->pkt_type, skb->skb_iif);
> > +
> > + if (dev)
> > + printk("%sdev name=%s feat=0x%pNF\n",
> > + level, dev->name, &dev->features);
> > + if (sk)
> > + printk("%ssk family=%hu type=%hu proto=%hu\n",
> > + level, sk->sk_family, sk->sk_type, sk->sk_protocol);
> > +
> > + if (full_pkt && headroom)
> > + print_hex_dump(level, "skb headroom: ", DUMP_PREFIX_OFFSET,
> > + 16, 1, skb->head, headroom, false);
> > +
> > + seg_len = min_t(int, skb_headlen(skb), len);
> > + if (seg_len)
> > + print_hex_dump(level, "skb linear: ", DUMP_PREFIX_OFFSET,
> > + 16, 1, skb->data, seg_len, false);
> > + len -= seg_len;
> > +
> > + if (full_pkt && tailroom)
> > + print_hex_dump(level, "skb tailroom: ", DUMP_PREFIX_OFFSET,
> > + 16, 1, skb_tail_pointer(skb), tailroom, false);
> > +
> > + for (i = 0; len && i < skb_shinfo(skb)->nr_frags; i++) {
> > + skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> > + u32 p_off, p_len, copied;
> > + struct page *p;
> > + u8 *vaddr;
> > +
> > + skb_frag_foreach_page(frag, frag->page_offset,
> > + skb_frag_size(frag), p, p_off, p_len,
> > + copied) {
> > + seg_len = min_t(int, p_len, len);
> > + vaddr = kmap_atomic(p);
> > + print_hex_dump(level, "skb frag: ",
> > + DUMP_PREFIX_OFFSET,
> > + 16, 1, vaddr + p_off, seg_len, false);
> > + kunmap_atomic(vaddr);
> > + len -= seg_len;
> > + if (!len)
> > + break;
> > + }
> > + }
> > +
> > + if (len && skb_has_frag_list(skb)) {
> > + printk("skb fraglist:\n");
> > + skb_walk_frags(skb, list_skb) {
> > + if (len <= 0)
> > + break;
> > + skb_dump(level, list_skb, len);
>
> Here we call skb_dump passing len as full_pkt.
>
> Maybe call it with skb_dump(level, list_skb, len, full_pkt);
>
> > + len -= list_skb->len;
> > + }
> > + }
> > +}
> > +EXPORT_SYMBOL(skb_dump);
^ permalink raw reply
* Re: [PATCH v2 bpf-next] bpf: cgroup: Fix build error without CONFIG_NET
From: Yonghong Song @ 2019-07-03 14:45 UTC (permalink / raw)
To: YueHaibing, ast@kernel.org, daniel@iogearbox.net, Martin Lau,
Song Liu, sdf@google.com
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
bpf@vger.kernel.org
In-Reply-To: <20190703082630.51104-1-yuehaibing@huawei.com>
On 7/3/19 1:26 AM, YueHaibing wrote:
> If CONFIG_NET is not set and CONFIG_CGROUP_BPF=y,
> gcc building fails:
>
> kernel/bpf/cgroup.o: In function `cg_sockopt_func_proto':
> cgroup.c:(.text+0x237e): undefined reference to `bpf_sk_storage_get_proto'
> cgroup.c:(.text+0x2394): undefined reference to `bpf_sk_storage_delete_proto'
> kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_getsockopt':
> (.text+0x2a1f): undefined reference to `lock_sock_nested'
> (.text+0x2ca2): undefined reference to `release_sock'
> kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_setsockopt':
> (.text+0x3006): undefined reference to `lock_sock_nested'
> (.text+0x32bb): undefined reference to `release_sock'
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Suggested-by: Stanislav Fomichev <sdf@fomichev.me>
> Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> v2: use ifdef macro
> ---
> kernel/bpf/cgroup.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 76fa007..0a00eac 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -939,6 +939,7 @@ int __cgroup_bpf_run_filter_sysctl(struct ctl_table_header *head,
> }
> EXPORT_SYMBOL(__cgroup_bpf_run_filter_sysctl);
>
> +#ifdef CONFIG_NET
> static bool __cgroup_bpf_prog_array_is_empty(struct cgroup *cgrp,
> enum bpf_attach_type attach_type)
> {
> @@ -1120,6 +1121,7 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
> return ret;
> }
> EXPORT_SYMBOL(__cgroup_bpf_run_filter_getsockopt);
> +#endif
>
> static ssize_t sysctl_cpy_dir(const struct ctl_dir *dir, char **bufp,
> size_t *lenp)
> @@ -1386,10 +1388,12 @@ static const struct bpf_func_proto *
> cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> {
> switch (func_id) {
> +#ifdef CONFIG_NET
> case BPF_FUNC_sk_storage_get:
> return &bpf_sk_storage_get_proto;
> case BPF_FUNC_sk_storage_delete:
> return &bpf_sk_storage_delete_proto;
> +#endif
> #ifdef CONFIG_INET
> case BPF_FUNC_tcp_sock:
> return &bpf_tcp_sock_proto;
>
^ permalink raw reply
* Re: [PATCH net-next v2 0/5] net: use ICW for sk_proto->{send,recv}msg
From: Willem de Bruijn @ 2019-07-03 14:45 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Network Development, David S. Miller
In-Reply-To: <cover.1562162469.git.pabeni@redhat.com>
On Wed, Jul 3, 2019 at 10:07 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> This series extends ICW usage to one of the few remaining spots in fast-path
> still hitting per packet retpoline overhead, namely the sk_proto->{send,recv}msg
> calls.
>
> The first 3 patches in this series refactor the existing code so that applying
> the ICW macros is straight-forward: we demux inet_{recv,send}msg in ipv4 and
> ipv6 variants so that each of them can easily select the appropriate TCP or UDP
> direct call. While at it, a new helper is created to avoid excessive code
> duplication, and the current ICWs for inet_{recv,send}msg are adjusted
> accordingly.
>
> The last 2 patches really introduce the new ICW use-case, respectively for the
> ipv6 and the ipv4 code path.
>
> This gives up to 5% performance improvement under UDP flood, and smaller but
> measurable gains for TCP RR workloads.
>
> v1 -> v2:
> - drop inet6_{recv,send}msg declaration from header file,
> prefer ICW macro instead
> - avoid unneeded reclaration for udp_sendmsg, as suggested by Willem
>
> Paolo Abeni (5):
> inet: factor out inet_send_prepare()
> ipv6: provide and use ipv6 specific version for {recv,send}msg
> net: adjust socket level ICW to cope with ipv6 variant of
> {recv,send}msg
> ipv6: use indirect call wrappers for {tcp,udpv6}_{recv,send}msg()
> ipv4: use indirect call wrappers for {tcp,udp}_{recv,send}msg()
Acked-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply
* Re: kernel panic: corrupted stack end in dput
From: Al Viro @ 2019-07-03 14:40 UTC (permalink / raw)
To: Hillf Danton
Cc: syzbot, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
syzkaller-bugs@googlegroups.com
In-Reply-To: <20190703064307.13740-1-hdanton@sina.com>
On Wed, Jul 03, 2019 at 02:43:07PM +0800, Hillf Danton wrote:
> > This is very much *NOT* fine.
> > 1) trylock can fail from any number of reasons, starting
> > with "somebody is going through the hash chain doing a lookup on
> > something completely unrelated"
>
> They are also a red light that we need to bail out of spiraling up
> the directory hierarchy imho.
Translation: "let's leak the reference to parent, shall we?"
> > 2) whoever had been holding the lock and whatever they'd
> > been doing might be over right after we get the return value from
> > spin_trylock().
>
> Or after we send a mail using git. I don't know.
>
> > 3) even had that been really somebody adding children in
> > the same parent *AND* even if they really kept doing that, rather
> > than unlocking and buggering off, would you care to explain why
> > dentry_unlist() called by __dentry_kill() and removing the victim
> > from the list of children would be safe to do in parallel with that?
> >
> My bad. I have to walk around that unsafety.
WHAT unsafety? Can you explain what are you seeing and how to
reproduce it, whatever it is?
^ permalink raw reply
* Re: [PATCH net] sctp: count data bundling sack chunk for outctrlchunks
From: Neil Horman @ 2019-07-03 14:38 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner
In-Reply-To: <62e917e312bc582e96fa19b502561e37ca7f91a6.1562149220.git.lucien.xin@gmail.com>
On Wed, Jul 03, 2019 at 06:20:20PM +0800, Xin Long wrote:
> Now all ctrl chunks are counted for asoc stats.octrlchunks and net
> SCTP_MIB_OUTCTRLCHUNKS either after queuing up or bundling, other
> than the chunk maked and bundled in sctp_packet_bundle_sack, which
> caused 'outctrlchunks' not consistent with 'inctrlchunks' in peer.
>
> This issue exists since very beginning, here to fix it by increasing
> both net SCTP_MIB_OUTCTRLCHUNKS and asoc stats.octrlchunks when sack
> chunk is maked and bundled in sctp_packet_bundle_sack.
>
> Reported-by: Ja Ram Jeon <jajeon@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/sctp/output.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index e0c2747..dbda7e7 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -282,6 +282,9 @@ static enum sctp_xmit sctp_packet_bundle_sack(struct sctp_packet *pkt,
> sctp_chunk_free(sack);
> goto out;
> }
> + SCTP_INC_STATS(sock_net(asoc->base.sk),
> + SCTP_MIB_OUTCTRLCHUNKS);
> + asoc->stats.octrlchunks++;
> asoc->peer.sack_needed = 0;
> if (del_timer(timer))
> sctp_association_put(asoc);
> --
> 2.1.0
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH net-next v6 06/15] ethtool: netlink bitset handling
From: Jiri Pirko @ 2019-07-03 14:37 UTC (permalink / raw)
To: Johannes Berg
Cc: Michal Kubecek, David Miller, netdev, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, linux-kernel
In-Reply-To: <b3cd61506080143f571d6286223ae33c8bd02c3a.camel@sipsolutions.net>
Wed, Jul 03, 2019 at 03:44:57PM CEST, johannes@sipsolutions.net wrote:
>On Wed, 2019-07-03 at 13:49 +0200, Jiri Pirko wrote:
>>
>> > +Value and mask must have length at least ETHTOOL_A_BITSET_SIZE bits rounded up
>> > +to a multiple of 32 bits. They consist of 32-bit words in host byte order,
>>
>> Looks like the blocks are similar to NLA_BITFIELD32. Why don't you user
>> nested array of NLA_BITFIELD32 instead?
>
>That would seem kind of awkward to use, IMHO.
>
>Perhaps better to make some kind of generic "arbitrary size bitfield"
>attribute type?
Yep, I believe I was trying to make this point during bitfield32
discussion, failed apparently. So if we have "NLA_BITFIELD" with
arbitrary size, that sounds good to me.
>
>Not really sure we want the complexity with _LIST and _SIZE, since you
>should always be able to express it as _VALUE and _MASK, right?
>
>Trying to think how we should express this best - bitfield32 is just a
>mask/value struct, for arbitrary size I guess we *could* just make it
>kind of a binary with arbitrary length that must be a multiple of 2
>bytes (or 2 u32-bit-words?) and then the first half is the value and the
>second half is the mask? Some more validation would be nicer, but having
>a generic attribute that actually is nested is awkward too.
>
>johannes
>
>
^ permalink raw reply
* Re: [PATCH net-next v6 08/15] ethtool: move string arrays into common file
From: Michal Kubecek @ 2019-07-03 14:37 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: <20190703134452.GZ2250@nanopsycho>
On Wed, Jul 03, 2019 at 03:44:52PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:50:19PM CEST, mkubecek@suse.cz wrote:
> >Introduce file net/ethtool/common.c for code shared by ioctl and netlink
> >ethtool interface. Move name tables of features, RSS hash functions,
> >tunables and PHY tunables into this file.
> >
> >Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> >---
> > net/ethtool/Makefile | 2 +-
> > net/ethtool/common.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
> > net/ethtool/common.h | 17 +++++++++
> > net/ethtool/ioctl.c | 83 ++-----------------------------------------
> > 4 files changed, 104 insertions(+), 82 deletions(-)
> > create mode 100644 net/ethtool/common.c
> > create mode 100644 net/ethtool/common.h
> >
> >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
> >index 482fdb9380fa..11782306593b 100644
> >--- a/net/ethtool/Makefile
> >+++ b/net/ethtool/Makefile
> >@@ -1,6 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0
> >
> >-obj-y += ioctl.o
> >+obj-y += ioctl.o common.o
> >
> > obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
> >
> >diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> >new file mode 100644
> >index 000000000000..b0ce420e994e
> >--- /dev/null
> >+++ b/net/ethtool/common.c
> >@@ -0,0 +1,84 @@
> >+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
> >+
> >+#include "common.h"
> >+
> >+const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
>
> const char *netdev_features_strings[NETDEV_FEATURE_COUNT] = {
> ?
>
> Same with the other arrays.
These are not new tables, this patch only moves existing tables from
ioctl.c (originally net/core/ethtool.c) into common.c so that they can
be used by both ioctl and netlink code.
This fixed size string array format is used by ETHTOOL_GSTRINGS ioctl
command. So if we switch these into simple const char *table[], we can
get rid of some complexity in strset.c and bitset.c (the "simple" vs.
"legacy" string set mess) but we would have to convert them into the
fixed size string array in ioctl ETHTOOL_GSTRINGS handler. And then we
would also have to convert (or rather "index") string sets retrieved
from NIC driver (e.g. private flags, stats, tests) - which also means an
extra kmalloc() (or rather kmalloc_array()).
It an option I'm certainly open to if we agree on it but it's not for
free.
Michal
^ permalink raw reply
* Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute
From: Jiri Pirko @ 2019-07-03 14:34 UTC (permalink / raw)
To: Andrew Lunn
Cc: Parav Pandit, Jakub Kicinski, Jiri Pirko, netdev@vger.kernel.org,
Saeed Mahameed, vivien.didelot, f.fainelli
In-Reply-To: <20190703140958.GB18473@lunn.ch>
Wed, Jul 03, 2019 at 04:09:58PM CEST, andrew@lunn.ch wrote:
>> However, we expose it for DEVLINK_PORT_FLAVOUR_CPU and
>> DEVLINK_PORT_FLAVOUR_DSA. Not sure if it makes sense there either.
>> Ccing Florian, Andrew and Vivien.
>> What do you guys think?
>
>Hi Jiri
>
>DSA and CPU ports are physical ports of the switch. And there can be
>multiple DSA ports, and maybe sometime real soon now, multiple CPU
>ports. So having a number associated with them is useful.
Okay. Makes sense.
>
> Andrew
^ permalink raw reply
* Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute
From: Jiri Pirko @ 2019-07-03 14:33 UTC (permalink / raw)
To: Parav Pandit
Cc: Jakub Kicinski, Jiri Pirko, netdev@vger.kernel.org,
Saeed Mahameed, vivien.didelot@gmail.com, andrew@lunn.ch,
f.fainelli@gmail.com
In-Reply-To: <AM0PR05MB4866675A8CB5BDB2890E14BBD1FB0@AM0PR05MB4866.eurprd05.prod.outlook.com>
Wed, Jul 03, 2019 at 03:49:51PM CEST, parav@mellanox.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Wednesday, July 3, 2019 4:07 PM
>> To: Parav Pandit <parav@mellanox.com>
>> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>; Jiri Pirko
>> <jiri@mellanox.com>; netdev@vger.kernel.org; Saeed Mahameed
>> <saeedm@mellanox.com>; vivien.didelot@gmail.com; andrew@lunn.ch;
>> f.fainelli@gmail.com
>> Subject: Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and
>> port attribute
>>
>> Wed, Jul 03, 2019 at 06:46:13AM CEST, parav@mellanox.com wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Jakub Kicinski <jakub.kicinski@netronome.com>
>> >> Sent: Wednesday, July 3, 2019 7:46 AM
>> >> To: Parav Pandit <parav@mellanox.com>
>> >> Cc: Jiri Pirko <jiri@mellanox.com>; netdev@vger.kernel.org; Saeed
>> >> Mahameed <saeedm@mellanox.com>
>> >> Subject: Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port
>> >> flavour and port attribute
>> >>
>> >> On Wed, 3 Jul 2019 02:08:39 +0000, Parav Pandit wrote:
>> >> > > If you want to expose some device specific eswitch port ID please
>> >> > > add a new attribute for that.
>> >> > > The fact that that ID may match port_number for your device today
>> >> > > is coincidental. port_number, and split attributes should not be
>> >> > > exposed for PCI ports.
>> >> >
>> >> > So your concern is non mellanox hw has eswitch but there may not be
>> >> > a unique handle to identify a eswitch port?
>> >>
>> >> That's not a concern, no. Like any debug attribute it should be optional.
>> >>
>> >> > Or that handle may be wider than 32-bit?
>> >>
>> >> 64 bit would probably be better, yes, although that wasn't my initial
>> >> concern.
>> >>
>> >Why 32-bit is not enough?
>> >
>> >> > And instead of treating port_number as handle, there should be
>> >> > different attribute, is that the ask?
>> >>
>> >> Yes, the ask, as always, is to not abuse existing attributes to carry
>> >> tangentially related information.
>> >
>> >Why it is tangential?
>> >Devlink_port has got a port_number. Depending on flavour this port_number
>> represents a port.
>> >If it is floavour=PHYSICAL, its physical port number.
>> >If it is eswitch pf/vf ports, it represents eswitch port.
>> >
>> >Why you see it only as physical_port_number?
>>
>> The original intention was like that. See the desc of
>> devlink_port_attrs_set():
>>
>> * @port_number: number of the port that is facing user, for example
>> * the front panel port number
>>
>> For vf/pf representors, this is not applicable and should be indeed avoided.
>>
>Physical port number is not applicable but this is useful information that completes the eswitch picture.
>Because eswitch has this termination end point anyway.
Use port_index. That is up to the driver to put whatever value there.
>Instead of inventing some new vendor specific field, I see value in using existing port_number field.
>Will wait for others inputs.
>
>> However, we expose it for DEVLINK_PORT_FLAVOUR_CPU and
>> DEVLINK_PORT_FLAVOUR_DSA. Not sure if it makes sense there either.
>> Ccing Florian, Andrew and Vivien.
>> What do you guys think?
>>
>> Perhaps we should have:
>> if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PHYSICAL &&
>> nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs-
>> >port_number))
>> return -EMSGSIZE;
>> in devlink_nl_port_attrs_put()
>
^ permalink raw reply
* Re: [PATCH net-next v6 09/15] ethtool: generic handlers for GET requests
From: Jiri Pirko @ 2019-07-03 14:25 UTC (permalink / raw)
To: Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <4faa0ce52dfe02c9cde5a46012b16c9af6764c5e.1562067622.git.mkubecek@suse.cz>
Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@suse.cz wrote:
[...]
>+/* generic ->doit() handler for GET type requests */
>+static int ethnl_get_doit(struct sk_buff *skb, struct genl_info *info)
It is very unfortunate for review to introduce function in a patch and
don't use it. In general, this approach is frowned upon. You should use
whatever you introduce in the same patch. I understand it is sometimes
hard.
IIUC, you have one ethnl_get_doit for all possible commands, and you
have this ops to do cmd-specific tasks. That is quite unusual. Plus if
you consider the complicated datastructures connected with this,
I'm lost from the beginning :( Any particular reason form this indirection?
I don't think any other generic netlink code does that (correct me if
I'm wrong). The nice thing about generic netlink is the fact that
you have separate handlers per cmd.
I don't think you need these ops and indirections. For the common parts,
just have a set of common helpers, as the other generic netlink users
are doing. The code would be much easier to read and follow then.
>+{
>+ const u8 cmd = info->genlhdr->cmd;
>+ const struct get_request_ops *ops;
>+ struct ethnl_req_info *req_info;
>+ struct sk_buff *rskb;
>+ void *reply_payload;
>+ int reply_len;
>+ int ret;
>+
>+ ops = get_requests[cmd];
>+ if (WARN_ONCE(!ops, "cmd %u has no get_request_ops\n", cmd))
>+ return -EOPNOTSUPP;
>+ req_info = ethnl_alloc_get_data(ops);
>+ if (!req_info)
>+ return -ENOMEM;
>+ ret = ethnl_std_parse(req_info, info->nlhdr, genl_info_net(info), ops,
>+ info->extack, !ops->allow_nodev_do);
>+ if (ret < 0)
>+ goto err_dev;
>+ req_info->privileged = ethnl_is_privileged(skb);
>+ ethnl_init_reply_data(req_info, ops, req_info->dev);
>+
>+ rtnl_lock();
>+ ret = ops->prepare_data(req_info, info);
>+ if (ret < 0)
>+ goto err_rtnl;
>+ reply_len = ops->reply_size(req_info);
>+ if (ret < 0)
>+ goto err_cleanup;
>+ ret = -ENOMEM;
>+ rskb = ethnl_reply_init(reply_len, req_info->dev, ops->reply_cmd,
>+ ops->hdr_attr, info, &reply_payload);
>+ if (!rskb)
>+ goto err_cleanup;
>+ ret = ops->fill_reply(rskb, req_info);
>+ if (ret < 0)
>+ goto err_msg;
>+ rtnl_unlock();
>+
>+ genlmsg_end(rskb, reply_payload);
>+ if (req_info->dev)
>+ dev_put(req_info->dev);
>+ ethnl_free_get_data(ops, req_info);
>+ return genlmsg_reply(rskb, info);
>+
>+err_msg:
>+ WARN_ONCE(ret == -EMSGSIZE,
>+ "calculated message payload length (%d) not sufficient\n",
>+ reply_len);
>+ nlmsg_free(rskb);
>+err_cleanup:
>+ ethnl_free_get_data(ops, req_info);
>+err_rtnl:
>+ rtnl_unlock();
>+err_dev:
>+ if (req_info->dev)
>+ dev_put(req_info->dev);
>+ return ret;
>+}
[...]
^ permalink raw reply
* Re: [PATCH net-next v6 07/15] ethtool: support for netlink notifications
From: Michal Kubecek @ 2019-07-03 14:18 UTC (permalink / raw)
To: netdev
Cc: Johannes Berg, David Miller, Jakub Kicinski, Jiri Pirko,
Andrew Lunn, Florian Fainelli, John Linville, Stephen Hemminger,
linux-kernel
In-Reply-To: <ea84d738346b96b81550e0fd0a6c715faa323061.camel@sipsolutions.net>
On Wed, Jul 03, 2019 at 03:39:54PM +0200, Johannes Berg wrote:
> On Tue, 2019-07-02 at 13:50 +0200, Michal Kubecek wrote:
> >
> > +static bool ethnl_ok __read_mostly;
>
> Not sure it makes a big difference, but it could probably be
> __ro_after_init instead?
Yes, that's more fitting; the flag is initialized to false, changes to
true once ethtool netlink is ready and never changes back. I wasn't
aware of __ro_after_init annotation.
Michal
^ permalink raw reply
* Re: [PATCH net-next v6 07/15] ethtool: support for netlink notifications
From: Michal Kubecek @ 2019-07-03 14:16 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: <20190703133352.GY2250@nanopsycho>
On Wed, Jul 03, 2019 at 03:33:52PM +0200, Jiri Pirko wrote:
> >+/* notifications */
> >+
> >+typedef void (*ethnl_notify_handler_t)(struct net_device *dev,
> >+ struct netlink_ext_ack *extack,
> >+ unsigned int cmd, u32 req_mask,
> >+ const void *data);
> >+
> >+static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
> >+};
> >+
> >+void ethtool_notify(struct net_device *dev, struct netlink_ext_ack *extack,
> >+ unsigned int cmd, u32 req_mask, const void *data)
>
> What's "req_mask" ?
It's infomask to interpret the same way as if it came from request
header (the notification triggered by a SET request or its ioctl
equivalent uses the same format as corresponding GET_REPLY message and
is created by the same code). But it could be called infomask, I have no
strong opinion about that.
> >+{
> >+ if (unlikely(!ethnl_ok))
> >+ return;
> >+ ASSERT_RTNL();
> >+
> >+ if (likely(cmd < ARRAY_SIZE(ethnl_notify_handlers) &&
> >+ ethnl_notify_handlers[cmd]))
>
> How it could be null?
Notification message types share the enum with other kernel messages:
/* message types - kernel to userspace */
enum {
ETHTOOL_MSG_KERNEL_NONE,
ETHTOOL_MSG_STRSET_GET_REPLY,
ETHTOOL_MSG_SETTINGS_GET_REPLY,
ETHTOOL_MSG_SETTINGS_NTF,
ETHTOOL_MSG_SETTINGS_SET_REPLY,
ETHTOOL_MSG_INFO_GET_REPLY,
ETHTOOL_MSG_PARAMS_GET_REPLY,
ETHTOOL_MSG_PARAMS_NTF,
ETHTOOL_MSG_NWAYRST_NTF,
ETHTOOL_MSG_PHYSID_NTF,
ETHTOOL_MSG_RESET_NTF,
ETHTOOL_MSG_RESET_ACT_REPLY,
ETHTOOL_MSG_RXFLOW_GET_REPLY,
ETHTOOL_MSG_RXFLOW_NTF,
ETHTOOL_MSG_RXFLOW_SET_REPLY,
/* add new constants above here */
__ETHTOOL_MSG_KERNEL_CNT,
ETHTOOL_MSG_KERNEL_MAX = (__ETHTOOL_MSG_KERNEL_CNT - 1)
};
Only entries for *_NTF types are non-null in ethnl_notify_handlers[]:
static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
[ETHTOOL_MSG_SETTINGS_NTF] = ethnl_std_notify,
[ETHTOOL_MSG_PARAMS_NTF] = ethnl_std_notify,
[ETHTOOL_MSG_NWAYRST_NTF] = ethnl_nwayrst_notify,
[ETHTOOL_MSG_PHYSID_NTF] = ethnl_physid_notify,
[ETHTOOL_MSG_RESET_NTF] = ethnl_reset_notify,
[ETHTOOL_MSG_RXFLOW_NTF] = ethnl_rxflow_notify,
};
If the check above fails, it means that kernel code tried to send
a notification with type which does not exist or is not a notification,
i.e. a bug in kernel; that's why the WARN_ONCE.
Michal
> >+ ethnl_notify_handlers[cmd](dev, extack, cmd, req_mask, data);
> >+ else
> >+ WARN_ONCE(1, "notification %u not implemented (dev=%s, req_mask=0x%x)\n",
> >+ cmd, netdev_name(dev), req_mask);
> >+}
> >+EXPORT_SYMBOL(ethtool_notify);
^ permalink raw reply
* Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute
From: Andrew Lunn @ 2019-07-03 14:09 UTC (permalink / raw)
To: Jiri Pirko
Cc: Parav Pandit, Jakub Kicinski, Jiri Pirko, netdev@vger.kernel.org,
Saeed Mahameed, vivien.didelot, f.fainelli
In-Reply-To: <20190703103720.GU2250@nanopsycho>
> However, we expose it for DEVLINK_PORT_FLAVOUR_CPU and
> DEVLINK_PORT_FLAVOUR_DSA. Not sure if it makes sense there either.
> Ccing Florian, Andrew and Vivien.
> What do you guys think?
Hi Jiri
DSA and CPU ports are physical ports of the switch. And there can be
multiple DSA ports, and maybe sometime real soon now, multiple CPU
ports. So having a number associated with them is useful.
Andrew
^ permalink raw reply
* [PATCH net-next v2 4/5] ipv6: use indirect call wrappers for {tcp,udpv6}_{recv,send}msg()
From: Paolo Abeni @ 2019-07-03 14:06 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Willem de Bruijn
In-Reply-To: <cover.1562162469.git.pabeni@redhat.com>
This avoids an indirect call per syscall for common ipv6 transports
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/ipv6/af_inet6.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 4d5ed473f722..ef37e0574f54 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -564,6 +564,8 @@ int inet6_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
}
EXPORT_SYMBOL(inet6_ioctl);
+INDIRECT_CALLABLE_DECLARE(int udpv6_sendmsg(struct sock *, struct msghdr *,
+ size_t));
int inet6_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
{
struct sock *sk = sock->sk;
@@ -571,9 +573,12 @@ int inet6_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
if (unlikely(inet_send_prepare(sk)))
return -EAGAIN;
- return sk->sk_prot->sendmsg(sk, msg, size);
+ return INDIRECT_CALL_2(sk->sk_prot->sendmsg, tcp_sendmsg, udpv6_sendmsg,
+ sk, msg, size);
}
+INDIRECT_CALLABLE_DECLARE(int udpv6_recvmsg(struct sock *, struct msghdr *,
+ size_t, int, int, int *));
int inet6_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
int flags)
{
@@ -584,8 +589,9 @@ int inet6_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
if (likely(!(flags & MSG_ERRQUEUE)))
sock_rps_record_flow(sk);
- err = sk->sk_prot->recvmsg(sk, msg, size, flags & MSG_DONTWAIT,
- flags & ~MSG_DONTWAIT, &addr_len);
+ err = INDIRECT_CALL_2(sk->sk_prot->recvmsg, tcp_recvmsg, udpv6_recvmsg,
+ sk, msg, size, flags & MSG_DONTWAIT,
+ flags & ~MSG_DONTWAIT, &addr_len);
if (err >= 0)
msg->msg_namelen = addr_len;
return err;
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v2 5/5] ipv4: use indirect call wrappers for {tcp,udp}_{recv,send}msg()
From: Paolo Abeni @ 2019-07-03 14:06 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Willem de Bruijn
In-Reply-To: <cover.1562162469.git.pabeni@redhat.com>
This avoids an indirect call per syscall for common ipv4 transports
v1 -> v2:
- avoid unneeded reclaration for udp_sendmsg, as suggested by Willem
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/ipv4/af_inet.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 8421e2f5bbb3..ed2301ef872e 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -804,7 +804,8 @@ int inet_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
if (unlikely(inet_send_prepare(sk)))
return -EAGAIN;
- return sk->sk_prot->sendmsg(sk, msg, size);
+ return INDIRECT_CALL_2(sk->sk_prot->sendmsg, tcp_sendmsg, udp_sendmsg,
+ sk, msg, size);
}
EXPORT_SYMBOL(inet_sendmsg);
@@ -822,6 +823,8 @@ ssize_t inet_sendpage(struct socket *sock, struct page *page, int offset,
}
EXPORT_SYMBOL(inet_sendpage);
+INDIRECT_CALLABLE_DECLARE(int udp_recvmsg(struct sock *, struct msghdr *,
+ size_t, int, int, int *));
int inet_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
int flags)
{
@@ -832,8 +835,9 @@ int inet_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
if (likely(!(flags & MSG_ERRQUEUE)))
sock_rps_record_flow(sk);
- err = sk->sk_prot->recvmsg(sk, msg, size, flags & MSG_DONTWAIT,
- flags & ~MSG_DONTWAIT, &addr_len);
+ err = INDIRECT_CALL_2(sk->sk_prot->recvmsg, tcp_recvmsg, udp_recvmsg,
+ sk, msg, size, flags & MSG_DONTWAIT,
+ flags & ~MSG_DONTWAIT, &addr_len);
if (err >= 0)
msg->msg_namelen = addr_len;
return err;
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v2 3/5] net: adjust socket level ICW to cope with ipv6 variant of {recv,send}msg
From: Paolo Abeni @ 2019-07-03 14:06 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Willem de Bruijn
In-Reply-To: <cover.1562162469.git.pabeni@redhat.com>
After the previous patch we have ipv{6,4} variants for {recv,send}msg,
we should use the generic _INET ICW variant to call into the proper
build-in.
This also allows dropping the now unused and rather ugly _INET4 ICW macro
v1 -> v2:
- use ICW macro to declare inet6_{recv,send}msg
- fix a couple of checkpatch offender in the code context
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/socket.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index 963df5dbdd54..a865708940f9 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -103,13 +103,6 @@
#include <net/busy_poll.h>
#include <linux/errqueue.h>
-/* proto_ops for ipv4 and ipv6 use the same {recv,send}msg function */
-#if IS_ENABLED(CONFIG_INET)
-#define INDIRECT_CALL_INET4(f, f1, ...) INDIRECT_CALL_1(f, f1, __VA_ARGS__)
-#else
-#define INDIRECT_CALL_INET4(f, f1, ...) f(__VA_ARGS__)
-#endif
-
#ifdef CONFIG_NET_RX_BUSY_POLL
unsigned int sysctl_net_busy_read __read_mostly;
unsigned int sysctl_net_busy_poll __read_mostly;
@@ -641,10 +634,13 @@ EXPORT_SYMBOL(__sock_tx_timestamp);
INDIRECT_CALLABLE_DECLARE(int inet_sendmsg(struct socket *, struct msghdr *,
size_t));
+INDIRECT_CALLABLE_DECLARE(int inet6_sendmsg(struct socket *, struct msghdr *,
+ size_t));
static inline int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg)
{
- int ret = INDIRECT_CALL_INET4(sock->ops->sendmsg, inet_sendmsg, sock,
- msg, msg_data_left(msg));
+ int ret = INDIRECT_CALL_INET(sock->ops->sendmsg, inet6_sendmsg,
+ inet_sendmsg, sock, msg,
+ msg_data_left(msg));
BUG_ON(ret == -EIOCBQUEUED);
return ret;
}
@@ -870,12 +866,15 @@ void __sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
EXPORT_SYMBOL_GPL(__sock_recv_ts_and_drops);
INDIRECT_CALLABLE_DECLARE(int inet_recvmsg(struct socket *, struct msghdr *,
- size_t , int ));
+ size_t, int));
+INDIRECT_CALLABLE_DECLARE(int inet6_recvmsg(struct socket *, struct msghdr *,
+ size_t, int));
static inline int sock_recvmsg_nosec(struct socket *sock, struct msghdr *msg,
int flags)
{
- return INDIRECT_CALL_INET4(sock->ops->recvmsg, inet_recvmsg, sock, msg,
- msg_data_left(msg), flags);
+ return INDIRECT_CALL_INET(sock->ops->recvmsg, inet6_recvmsg,
+ inet_recvmsg, sock, msg, msg_data_left(msg),
+ flags);
}
/**
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v2 2/5] ipv6: provide and use ipv6 specific version for {recv,send}msg
From: Paolo Abeni @ 2019-07-03 14:06 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Willem de Bruijn
In-Reply-To: <cover.1562162469.git.pabeni@redhat.com>
This will simplify indirect call wrapper invocation in the following
patch.
No functional change intended, any - out-of-tree - IPv6 user of
inet_{recv,send}msg can keep using the existing functions.
SCTP code still uses the existing version even for ipv6: as this series
will not add ICW for SCTP, moving to the new helper would not give
any benefit.
The only other in-kernel user of inet_{recv,send}msg is
pvcalls_conn_back_read(), but psvcalls explicitly creates only IPv4 socket,
so no need to update that code path, too.
v1 -> v2: drop inet6_{recv,send}msg declaration from header file,
prefer ICW macro instead
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/ipv6/af_inet6.c | 35 +++++++++++++++++++++++++++++++----
1 file changed, 31 insertions(+), 4 deletions(-)
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 8369af32cef6..4d5ed473f722 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -564,6 +564,33 @@ int inet6_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
}
EXPORT_SYMBOL(inet6_ioctl);
+int inet6_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
+{
+ struct sock *sk = sock->sk;
+
+ if (unlikely(inet_send_prepare(sk)))
+ return -EAGAIN;
+
+ return sk->sk_prot->sendmsg(sk, msg, size);
+}
+
+int inet6_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ int addr_len = 0;
+ int err;
+
+ if (likely(!(flags & MSG_ERRQUEUE)))
+ sock_rps_record_flow(sk);
+
+ err = sk->sk_prot->recvmsg(sk, msg, size, flags & MSG_DONTWAIT,
+ flags & ~MSG_DONTWAIT, &addr_len);
+ if (err >= 0)
+ msg->msg_namelen = addr_len;
+ return err;
+}
+
const struct proto_ops inet6_stream_ops = {
.family = PF_INET6,
.owner = THIS_MODULE,
@@ -580,8 +607,8 @@ const struct proto_ops inet6_stream_ops = {
.shutdown = inet_shutdown, /* ok */
.setsockopt = sock_common_setsockopt, /* ok */
.getsockopt = sock_common_getsockopt, /* ok */
- .sendmsg = inet_sendmsg, /* ok */
- .recvmsg = inet_recvmsg, /* ok */
+ .sendmsg = inet6_sendmsg, /* retpoline's sake */
+ .recvmsg = inet6_recvmsg, /* retpoline's sake */
#ifdef CONFIG_MMU
.mmap = tcp_mmap,
#endif
@@ -614,8 +641,8 @@ const struct proto_ops inet6_dgram_ops = {
.shutdown = inet_shutdown, /* ok */
.setsockopt = sock_common_setsockopt, /* ok */
.getsockopt = sock_common_getsockopt, /* ok */
- .sendmsg = inet_sendmsg, /* ok */
- .recvmsg = inet_recvmsg, /* ok */
+ .sendmsg = inet6_sendmsg, /* retpoline's sake */
+ .recvmsg = inet6_recvmsg, /* retpoline's sake */
.mmap = sock_no_mmap,
.sendpage = sock_no_sendpage,
.set_peek_off = sk_set_peek_off,
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v2 1/5] inet: factor out inet_send_prepare()
From: Paolo Abeni @ 2019-07-03 14:06 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Willem de Bruijn
In-Reply-To: <cover.1562162469.git.pabeni@redhat.com>
The same code is replicated verbatim in multiple places, and the next
patches will introduce an additional user for it. Factor out a
helper and use it where appropriate. No functional change intended.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/net/inet_common.h | 1 +
net/ipv4/af_inet.c | 21 +++++++++++++--------
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/include/net/inet_common.h b/include/net/inet_common.h
index 975901a95c0f..ae2ba897675c 100644
--- a/include/net/inet_common.h
+++ b/include/net/inet_common.h
@@ -25,6 +25,7 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr,
int addr_len, int flags);
int inet_accept(struct socket *sock, struct socket *newsock, int flags,
bool kern);
+int inet_send_prepare(struct sock *sk);
int inet_sendmsg(struct socket *sock, struct msghdr *msg, size_t size);
ssize_t inet_sendpage(struct socket *sock, struct page *page, int offset,
size_t size, int flags);
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 52bdb881a506..8421e2f5bbb3 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -784,10 +784,8 @@ int inet_getname(struct socket *sock, struct sockaddr *uaddr,
}
EXPORT_SYMBOL(inet_getname);
-int inet_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
+int inet_send_prepare(struct sock *sk)
{
- struct sock *sk = sock->sk;
-
sock_rps_record_flow(sk);
/* We may need to bind the socket. */
@@ -795,6 +793,17 @@ int inet_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
inet_autobind(sk))
return -EAGAIN;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(inet_send_prepare);
+
+int inet_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
+{
+ struct sock *sk = sock->sk;
+
+ if (unlikely(inet_send_prepare(sk)))
+ return -EAGAIN;
+
return sk->sk_prot->sendmsg(sk, msg, size);
}
EXPORT_SYMBOL(inet_sendmsg);
@@ -804,11 +813,7 @@ ssize_t inet_sendpage(struct socket *sock, struct page *page, int offset,
{
struct sock *sk = sock->sk;
- sock_rps_record_flow(sk);
-
- /* We may need to bind the socket. */
- if (!inet_sk(sk)->inet_num && !sk->sk_prot->no_autobind &&
- inet_autobind(sk))
+ if (unlikely(inet_send_prepare(sk)))
return -EAGAIN;
if (sk->sk_prot->sendpage)
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v2 0/5] net: use ICW for sk_proto->{send,recv}msg
From: Paolo Abeni @ 2019-07-03 14:06 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Willem de Bruijn
This series extends ICW usage to one of the few remaining spots in fast-path
still hitting per packet retpoline overhead, namely the sk_proto->{send,recv}msg
calls.
The first 3 patches in this series refactor the existing code so that applying
the ICW macros is straight-forward: we demux inet_{recv,send}msg in ipv4 and
ipv6 variants so that each of them can easily select the appropriate TCP or UDP
direct call. While at it, a new helper is created to avoid excessive code
duplication, and the current ICWs for inet_{recv,send}msg are adjusted
accordingly.
The last 2 patches really introduce the new ICW use-case, respectively for the
ipv6 and the ipv4 code path.
This gives up to 5% performance improvement under UDP flood, and smaller but
measurable gains for TCP RR workloads.
v1 -> v2:
- drop inet6_{recv,send}msg declaration from header file,
prefer ICW macro instead
- avoid unneeded reclaration for udp_sendmsg, as suggested by Willem
Paolo Abeni (5):
inet: factor out inet_send_prepare()
ipv6: provide and use ipv6 specific version for {recv,send}msg
net: adjust socket level ICW to cope with ipv6 variant of
{recv,send}msg
ipv6: use indirect call wrappers for {tcp,udpv6}_{recv,send}msg()
ipv4: use indirect call wrappers for {tcp,udp}_{recv,send}msg()
include/net/inet_common.h | 1 +
net/ipv4/af_inet.c | 31 ++++++++++++++++++-----------
net/ipv6/af_inet6.c | 41 +++++++++++++++++++++++++++++++++++----
net/socket.c | 23 +++++++++++-----------
4 files changed, 69 insertions(+), 27 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support
From: Andrew Lunn @ 2019-07-03 14:05 UTC (permalink / raw)
To: Voon Weifeng
Cc: David S. Miller, Maxime Coquelin, netdev, linux-kernel,
Jose Abreu, Giuseppe Cavallaro, Florian Fainelli,
Alexandre Torgue, biao huang, Ong Boon Leong, Kweh Hock Leong
In-Reply-To: <1562147404-4371-1-git-send-email-weifeng.voon@intel.com>
On Wed, Jul 03, 2019 at 05:50:04PM +0800, Voon Weifeng wrote:
> @@ -155,22 +171,26 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
> struct stmmac_priv *priv = netdev_priv(ndev);
> unsigned int mii_address = priv->hw->mii.addr;
> unsigned int mii_data = priv->hw->mii.data;
> - u32 v;
> - int data;
> u32 value = MII_BUSY;
> + int data = 0;
> + u32 v;
>
> value |= (phyaddr << priv->hw->mii.addr_shift)
> & priv->hw->mii.addr_mask;
> value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw->mii.reg_mask;
> value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
> & priv->hw->mii.clk_csr_mask;
> - if (priv->plat->has_gmac4)
> + if (priv->plat->has_gmac4) {
> value |= MII_GMAC4_READ;
> + if (phyreg & MII_ADDR_C45)
> + stmmac_mdio_c45_setup(priv, phyreg, &value, &data);
> + }
>
> if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
> 100, 10000))
> return -EBUSY;
>
> + writel(data, priv->ioaddr + mii_data);
That looks odd. Could you explain why it is needed.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH net-next v5 0/5] Add MPLS actions to TC
From: Willem de Bruijn @ 2019-07-03 14:02 UTC (permalink / raw)
To: John Hurley
Cc: Network Development, David Miller, Jiri Pirko, Cong Wang,
David Ahern, Simon Horman, Jakub Kicinski, oss-drivers
In-Reply-To: <CAK+XE==_AahZczgb4hU9auoj8=Kcx66JEdK3ZQ3TYpQuxdT05A@mail.gmail.com>
On Wed, Jul 3, 2019 at 4:33 AM John Hurley <john.hurley@netronome.com> wrote:
>
> On Wed, Jul 3, 2019 at 3:19 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Tue, Jul 2, 2019 at 8:32 PM John Hurley <john.hurley@netronome.com> wrote:
> > >
> > > This patchset introduces a new TC action module that allows the
> > > manipulation of the MPLS headers of packets. The code impliments
> > > functionality including push, pop, and modify.
> > >
> > > Also included are tests for the new funtionality. Note that these will
> > > require iproute2 changes to be submitted soon.
> > >
> > > NOTE: these patches are applied to net-next along with the patch:
> > > [PATCH net 1/1] net: openvswitch: fix csum updates for MPLS actions
> > > This patch has been accepted into net but, at time of posting, is not yet
> > > in net-next.
> > >
> > > v4-v5:
> > > - move mpls_hdr() call to after skb_ensure_writable - patch 3
> > > (Willem de Bruijn)
> > > - move mpls_dec_ttl to helper - patch 4 (Willem de Bruijn)
> > > - add iproute2 usage example to commit msg - patch 4 (David Ahern)
> > > - align label validation with mpls core code - patch 4 (David Ahern)
> > > - improve extack message for no proto in mpls pop - patch 4 (David Ahern)
> > > v3-v4:
> > > - refactor and reuse OvS code (Cong Wang)
> > > - use csum API rather than skb_post*rscum to update skb->csum (Cong Wang)
> > > - remove unnecessary warning (Cong Wang)
> > > - add comments to uapi attributes (David Ahern)
> > > - set strict type policy check for TCA_MPLS_UNSPEC (David Ahern)
> > > - expand/improve extack messages (David Ahern)
> > > - add option to manually set BOS
> > > v2-v3:
> > > - remove a few unnecessary line breaks (Jiri Pirko)
> > > - retract hw offload patch from set (resubmit with driver changes) (Jiri)
> > > v1->v2:
> > > - ensure TCA_ID_MPLS does not conflict with TCA_ID_CTINFO (Davide Caratti)
> > >
> > > John Hurley (5):
> > > net: core: move push MPLS functionality from OvS to core helper
> > > net: core: move pop MPLS functionality from OvS to core helper
> > > net: core: add MPLS update core helper and use in OvS
> > > net: sched: add mpls manipulation actions to TC
> > > selftests: tc-tests: actions: add MPLS tests
> >
> > Reviewed-by: Willem de Bruijn <willemb@google.com>
> >
> > I did have some conflicts applying the patches from patchwork (to diff
> > v4 vs v5). Might be my process. This is clean against net-next, right?
>
> Hi Willem, thanks for review.
> See the note in the cover letter....
> We had a patch accepted into net earlier in the week, these patches
> are applied to net-next + that patch.
> Unfortunately when we applied the patches direct to net-next and tried
> to merge in net then we got merge conflicts that needed manually
> fixed.
> Basically, the above patches should apply cleanly to net-next once net
> has been merged in.
Ah, that explains. Excellent, thanks for the follow-up. I had missed
that point in the cover letter (clearly).
^ permalink raw reply
* RE: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute
From: Parav Pandit @ 2019-07-03 13:49 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, Jiri Pirko, netdev@vger.kernel.org,
Saeed Mahameed, vivien.didelot@gmail.com, andrew@lunn.ch,
f.fainelli@gmail.com
In-Reply-To: <20190703103720.GU2250@nanopsycho>
> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Wednesday, July 3, 2019 4:07 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>; Jiri Pirko
> <jiri@mellanox.com>; netdev@vger.kernel.org; Saeed Mahameed
> <saeedm@mellanox.com>; vivien.didelot@gmail.com; andrew@lunn.ch;
> f.fainelli@gmail.com
> Subject: Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and
> port attribute
>
> Wed, Jul 03, 2019 at 06:46:13AM CEST, parav@mellanox.com wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> >> Sent: Wednesday, July 3, 2019 7:46 AM
> >> To: Parav Pandit <parav@mellanox.com>
> >> Cc: Jiri Pirko <jiri@mellanox.com>; netdev@vger.kernel.org; Saeed
> >> Mahameed <saeedm@mellanox.com>
> >> Subject: Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port
> >> flavour and port attribute
> >>
> >> On Wed, 3 Jul 2019 02:08:39 +0000, Parav Pandit wrote:
> >> > > If you want to expose some device specific eswitch port ID please
> >> > > add a new attribute for that.
> >> > > The fact that that ID may match port_number for your device today
> >> > > is coincidental. port_number, and split attributes should not be
> >> > > exposed for PCI ports.
> >> >
> >> > So your concern is non mellanox hw has eswitch but there may not be
> >> > a unique handle to identify a eswitch port?
> >>
> >> That's not a concern, no. Like any debug attribute it should be optional.
> >>
> >> > Or that handle may be wider than 32-bit?
> >>
> >> 64 bit would probably be better, yes, although that wasn't my initial
> >> concern.
> >>
> >Why 32-bit is not enough?
> >
> >> > And instead of treating port_number as handle, there should be
> >> > different attribute, is that the ask?
> >>
> >> Yes, the ask, as always, is to not abuse existing attributes to carry
> >> tangentially related information.
> >
> >Why it is tangential?
> >Devlink_port has got a port_number. Depending on flavour this port_number
> represents a port.
> >If it is floavour=PHYSICAL, its physical port number.
> >If it is eswitch pf/vf ports, it represents eswitch port.
> >
> >Why you see it only as physical_port_number?
>
> The original intention was like that. See the desc of
> devlink_port_attrs_set():
>
> * @port_number: number of the port that is facing user, for example
> * the front panel port number
>
> For vf/pf representors, this is not applicable and should be indeed avoided.
>
Physical port number is not applicable but this is useful information that completes the eswitch picture.
Because eswitch has this termination end point anyway.
Instead of inventing some new vendor specific field, I see value in using existing port_number field.
Will wait for others inputs.
> However, we expose it for DEVLINK_PORT_FLAVOUR_CPU and
> DEVLINK_PORT_FLAVOUR_DSA. Not sure if it makes sense there either.
> Ccing Florian, Andrew and Vivien.
> What do you guys think?
>
> Perhaps we should have:
> if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PHYSICAL &&
> nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs-
> >port_number))
> return -EMSGSIZE;
> in devlink_nl_port_attrs_put()
^ permalink raw reply
* Re: [PATCH net-next v6 06/15] ethtool: netlink bitset handling
From: Johannes Berg @ 2019-07-03 13:44 UTC (permalink / raw)
To: Jiri Pirko, Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, linux-kernel
In-Reply-To: <20190703114933.GW2250@nanopsycho>
On Wed, 2019-07-03 at 13:49 +0200, Jiri Pirko wrote:
>
> > +Value and mask must have length at least ETHTOOL_A_BITSET_SIZE bits rounded up
> > +to a multiple of 32 bits. They consist of 32-bit words in host byte order,
>
> Looks like the blocks are similar to NLA_BITFIELD32. Why don't you user
> nested array of NLA_BITFIELD32 instead?
That would seem kind of awkward to use, IMHO.
Perhaps better to make some kind of generic "arbitrary size bitfield"
attribute type?
Not really sure we want the complexity with _LIST and _SIZE, since you
should always be able to express it as _VALUE and _MASK, right?
Trying to think how we should express this best - bitfield32 is just a
mask/value struct, for arbitrary size I guess we *could* just make it
kind of a binary with arbitrary length that must be a multiple of 2
bytes (or 2 u32-bit-words?) and then the first half is the value and the
second half is the mask? Some more validation would be nicer, but having
a generic attribute that actually is nested is awkward too.
johannes
^ 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