* [net-next 0/4] Add gve driver
From: Catherine Sullivan @ 2019-06-26 18:52 UTC (permalink / raw)
To: netdev; +Cc: Catherine Sullivan
This patch series adds the gve driver which will support the
Compute Engine Virtual NIC that will be available in the future.
Catherine Sullivan (4):
gve: Add basic driver framework for Compute Engine Virtual NIC
gve: Add transmit and receive support
gve: Add workqueue and reset support
gve: Add ethtool support
.../networking/device_drivers/google/gve.rst | 123 ++
.../networking/device_drivers/index.rst | 1 +
MAINTAINERS | 9 +
drivers/net/ethernet/Kconfig | 1 +
drivers/net/ethernet/Makefile | 1 +
drivers/net/ethernet/google/Kconfig | 27 +
drivers/net/ethernet/google/Makefile | 5 +
drivers/net/ethernet/google/gve/Makefile | 4 +
drivers/net/ethernet/google/gve/gve.h | 456 ++++++
drivers/net/ethernet/google/gve/gve_adminq.c | 389 ++++++
drivers/net/ethernet/google/gve/gve_adminq.h | 215 +++
drivers/net/ethernet/google/gve/gve_desc.h | 118 ++
drivers/net/ethernet/google/gve/gve_ethtool.c | 226 +++
drivers/net/ethernet/google/gve/gve_main.c | 1231 +++++++++++++++++
.../net/ethernet/google/gve/gve_register.h | 27 +
drivers/net/ethernet/google/gve/gve_rx.c | 445 ++++++
.../net/ethernet/google/gve/gve_size_assert.h | 15 +
drivers/net/ethernet/google/gve/gve_tx.c | 584 ++++++++
18 files changed, 3877 insertions(+)
create mode 100644 Documentation/networking/device_drivers/google/gve.rst
create mode 100644 drivers/net/ethernet/google/Kconfig
create mode 100644 drivers/net/ethernet/google/Makefile
create mode 100644 drivers/net/ethernet/google/gve/Makefile
create mode 100644 drivers/net/ethernet/google/gve/gve.h
create mode 100644 drivers/net/ethernet/google/gve/gve_adminq.c
create mode 100644 drivers/net/ethernet/google/gve/gve_adminq.h
create mode 100644 drivers/net/ethernet/google/gve/gve_desc.h
create mode 100644 drivers/net/ethernet/google/gve/gve_ethtool.c
create mode 100644 drivers/net/ethernet/google/gve/gve_main.c
create mode 100644 drivers/net/ethernet/google/gve/gve_register.h
create mode 100644 drivers/net/ethernet/google/gve/gve_rx.c
create mode 100644 drivers/net/ethernet/google/gve/gve_size_assert.h
create mode 100644 drivers/net/ethernet/google/gve/gve_tx.c
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: INFO: rcu detected stall in ext4_write_checks
From: Theodore Ts'o @ 2019-06-26 18:42 UTC (permalink / raw)
To: syzbot
Cc: adilger.kernel, davem, eladr, idosch, jiri, john.stultz,
linux-ext4, linux-kernel, netdev, syzkaller-bugs, tglx
In-Reply-To: <000000000000d3f34b058c3d5a4f@google.com>
On Wed, Jun 26, 2019 at 10:27:08AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: abf02e29 Merge tag 'pm-5.2-rc6' of git://git.kernel.org/pu..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=1435aaf6a00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=e5c77f8090a3b96b
> dashboard link: https://syzkaller.appspot.com/bug?extid=4bfbbf28a2e50ab07368
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11234c41a00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15d7f026a00000
>
> The bug was bisected to:
>
> commit 0c81ea5db25986fb2a704105db454a790c59709c
> Author: Elad Raz <eladr@mellanox.com>
> Date: Fri Oct 28 19:35:58 2016 +0000
>
> mlxsw: core: Add port type (Eth/IB) set API
Um, so this doesn't pass the laugh test.
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10393a89a00000
It looks like the automated bisection machinery got confused by two
failures getting triggered by the same repro; the symptoms changed
over time. Initially, the failure was:
crashed: INFO: rcu detected stall in {sys_sendfile64,ext4_file_write_iter}
Later, the failure changed to something completely different, and much
earlier (before the test was even started):
run #5: basic kernel testing failed: failed to copy test binary to VM: failed to run ["scp" "-P" "22" "-F" "/dev/null" "-o" "UserKnownHostsFile=/dev/null" "-o" "BatchMode=yes" "-o" "IdentitiesOnly=yes" "-o" "StrictHostKeyChecking=no" "-o" "ConnectTimeout=10" "-i" "/syzkaller/jobs/linux/workdir/image/key" "/tmp/syz-executor216456474" "root@10.128.15.205:./syz-executor216456474"]: exit status 1
Connection timed out during banner exchange
lost connection
Looks like an opportunity to improve the bisection engine?
- Ted
^ permalink raw reply
* Re: [PATCH v2 bpf-next 0/3] libbpf: add perf buffer abstraction and API
From: Andrii Nakryiko @ 2019-06-26 18:41 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, bpf,
Networking, Kernel Team
In-Reply-To: <877e98d0hp.fsf@toke.dk>
On Wed, Jun 26, 2019 at 4:55 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andriin@fb.com> writes:
>
> > This patchset adds a high-level API for setting up and polling perf buffers
> > associated with BPF_MAP_TYPE_PERF_EVENT_ARRAY map. Details of APIs are
> > described in corresponding commit.
> >
> > Patch #1 adds a set of APIs to set up and work with perf buffer.
> > Patch #2 enhances libbpf to supprot auto-setting PERF_EVENT_ARRAY map size.
> > Patch #3 adds test.
>
> Having this in libbpf is great! Do you have a usage example of how a
> program is supposed to read events from the buffer? This is something we
> would probably want to add to the XDP tutorial
Did you check patch #3 with selftest? It's essentially an end-to-end
example of how to set everything up and process data (in my case it's
just simple int being sent as a sample, but it's exactly the same with
more complicated structs). I didn't bother to handle lost samples
notification, but it's just another optional callback with a single
counter denoting how many samples were dropped.
Let me know if it's still unclear.
>
> -Toke
^ permalink raw reply
* Re: [PATCH 2/3 nf-next] netfilter:nf_flow_table: Support bridge type flow offload
From: Pablo Neira Ayuso @ 2019-06-26 18:40 UTC (permalink / raw)
To: wenxu; +Cc: fw, netfilter-devel, netdev
In-Reply-To: <1561545148-11978-2-git-send-email-wenxu@ucloud.cn>
On Wed, Jun 26, 2019 at 06:32:27PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> With nf_conntrack_bridge function. The bridge family can do
> conntrack it self. The flow offload function based on the
> conntrack. So the flow in the bridge wih conntrack can be
> offloaded.
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
> include/net/netfilter/nf_flow_table.h | 30 +++++++++++-
> net/netfilter/nf_flow_table_core.c | 53 ++++++++++++++++-----
> net/netfilter/nf_flow_table_ip.c | 41 +++++++++++++---
> net/netfilter/nft_flow_offload.c | 89 ++++++++++++++++++++++++++++++++---
> 4 files changed, 185 insertions(+), 28 deletions(-)
>
> diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
> index 968be64..9a0cf27 100644
> --- a/include/net/netfilter/nf_flow_table.h
> +++ b/include/net/netfilter/nf_flow_table.h
> @@ -33,8 +33,22 @@ enum flow_offload_tuple_dir {
> FLOW_OFFLOAD_DIR_MAX = IP_CT_DIR_MAX
> };
>
> +enum flow_offload_tuple_type {
> + FLOW_OFFLOAD_TYPE_INET,
> + FLOW_OFFLOAD_TYPE_BRIDGE,
> +};
> +
> +struct dst_br_port {
> + struct net_device *dev;
> + u16 dst_vlan_tag;
> +};
> +
> struct flow_offload_dst {
> - struct dst_entry *dst_cache;
> + enum flow_offload_tuple_type type;
> + union {
> + struct dst_entry *dst_cache;
> + struct dst_br_port dst_port;
> + };
> };
>
> struct flow_offload_tuple {
> @@ -52,6 +66,7 @@ struct flow_offload_tuple {
> };
>
> int iifidx;
> + u16 vlan_tag;
>
> u8 l3proto;
> u8 l4proto;
> @@ -89,8 +104,19 @@ struct nf_flow_route {
> } tuple[FLOW_OFFLOAD_DIR_MAX];
> };
>
> +struct nf_flow_forward {
> + struct {
> + struct dst_br_port dst_port;
> + u16 vlan_tag;
> + } tuple[FLOW_OFFLOAD_DIR_MAX];
> +};
> +
> struct nf_flow_data {
> - struct nf_flow_route route;
> + enum flow_offload_tuple_type type;
> + union {
> + struct nf_flow_route route;
> + struct nf_flow_forward forward;
> + };
> };
>
> struct flow_offload *flow_offload_alloc(struct nf_conn *ct,
> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
> index 125ce1c..19ee69c 100644
> --- a/net/netfilter/nf_flow_table_core.c
> +++ b/net/netfilter/nf_flow_table_core.c
> @@ -29,16 +29,38 @@ struct flow_offload_entry {
> {
> struct flow_offload_tuple *ft = &flow->tuplehash[dir].tuple;
> struct nf_conntrack_tuple *ctt = &ct->tuplehash[dir].tuple;
> - struct dst_entry *other_dst = date->route.tuple[!dir].dst;
> - struct dst_entry *dst = data->route.tuple[dir].dst;
>
> + struct dst_entry *other_dst;
> + struct dst_entry *dst;
> + struct dst_br_port other_dst_port;
> + struct dst_br_port dst_port;
Please, reverse xmas tree for variable definitions.
> + if (data->type == FLOW_OFFLOAD_TYPE_BRIDGE) {
Could you add functions for these?
nf_flow_fill_bridge_dst(...)
> + other_dst_port = data->forward.tuple[!dir].dst_port;
> + dst_port = data->forward.tuple[dir].dst_port;
> +
> + ft->iifidx = other_dst_port.dev->ifindex;
> + ft->dst.dst_port = dst_port;
> + ft->vlan_tag = data->forward.tuple[dir].vlan_tag;
> + } else {
You could probably make an initial patch to add this function, so this
patch becomes smaller and easier to review:
nf_flow_fill_inet_dst(...)
to wrap the code below.
> + other_dst = data->route.tuple[!dir].dst;
> + dst = data->route.tuple[dir].dst;
> +
> + ft->iifidx = other_dst->dev->ifindex;
> + ft->dst.dst_cache = dst;
> + }
> +
> + ft->dst.type = data->type;
> ft->dir = dir;
>
> switch (ctt->src.l3num) {
> case NFPROTO_IPV4:
> ft->src_v4 = ctt->src.u3.in;
> ft->dst_v4 = ctt->dst.u3.in;
> - ft->mtu = ip_dst_mtu_maybe_forward(dst, true);
> + if (data->type == FLOW_OFFLOAD_TYPE_BRIDGE)
> + ft->mtu = dst_port.dev->mtu;
> + else
> + ft->mtu = ip_dst_mtu_maybe_forward(dst, true);
> break;
> case NFPROTO_IPV6:
> ft->src_v6 = ctt->src.u3.in6;
> @@ -51,9 +73,6 @@ struct flow_offload_entry {
> ft->l4proto = ctt->dst.protonum;
> ft->src_port = ctt->src.u.tcp.port;
> ft->dst_port = ctt->dst.u.tcp.port;
> -
> - ft->iifidx = other_dst->dev->ifindex;
> - ft->dst_cache = dst;
> }
>
> struct flow_offload *
> @@ -72,11 +91,13 @@ struct flow_offload *
>
> flow = &entry->flow;
>
> - if (!dst_hold_safe(data->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
> - goto err_dst_cache_original;
> + if (data->type == FLOW_OFFLOAD_TYPE_INET) {
Place this code below in a function?
> + if (!dst_hold_safe(data->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
> + goto err_dst_cache_original;
>
> - if (!dst_hold_safe(data->route.tuple[FLOW_OFFLOAD_DIR_REPLY].dst))
> - goto err_dst_cache_reply;
> + if (!dst_hold_safe(data->route.tuple[FLOW_OFFLOAD_DIR_REPLY].dst))
> + goto err_dst_cache_reply;
> + }
>
> entry->ct = ct;
>
> @@ -91,7 +112,8 @@ struct flow_offload *
> return flow;
>
> err_dst_cache_reply:
> - dst_release(data->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
> + if (data->type == FLOW_OFFLOAD_TYPE_INET)
> + dst_release(data->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
Same thing here, place this code in a function.
> err_dst_cache_original:
> kfree(entry);
> err_ct_refcnt:
> @@ -139,8 +161,13 @@ void flow_offload_free(struct flow_offload *flow)
> {
> struct flow_offload_entry *e;
>
> - dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_cache);
> - dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_cache);
> + if (flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.type == FLOW_OFFLOAD_TYPE_INET) {
Place this code in a function.
Better use switch() rather than if().
> + dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_cache);
> + dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_cache);
> + } else {
> + dev_put(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_port.dev);
> + dev_put(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_port.dev);
> + }
> e = container_of(flow, struct flow_offload_entry, flow);
> if (flow->flags & FLOW_OFFLOAD_DYING)
> nf_ct_delete(e->ct, 0, 0);
> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index 0016bb8..9af01ef 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -16,6 +16,8 @@
> #include <linux/tcp.h>
> #include <linux/udp.h>
>
> +#include "../bridge/br_private.h"
> +
> static int nf_flow_state_check(struct flow_offload *flow, int proto,
> struct sk_buff *skb, unsigned int thoff)
> {
> @@ -220,6 +222,7 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
> {
> struct flow_offload_tuple_rhash *tuplehash;
> struct nf_flowtable *flow_table = priv;
> + int family = flow_table->type->family;
> struct flow_offload_tuple tuple = {};
> enum flow_offload_tuple_dir dir;
> struct flow_offload *flow;
> @@ -228,6 +231,7 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
> unsigned int thoff;
> struct iphdr *iph;
> __be32 nexthop;
> + u16 vlan_tag;
>
> if (skb->protocol != htons(ETH_P_IP))
> return NF_ACCEPT;
> @@ -235,14 +239,25 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
> if (nf_flow_tuple_ip(skb, state->in, &tuple) < 0)
> return NF_ACCEPT;
>
> + if (family != NFPROTO_BRIDGE && family != NFPROTO_IPV4)
> + return NF_ACCEPT;
> +
> + if (family == NFPROTO_BRIDGE && skb_vlan_tag_present(skb))
> + tuple.vlan_tag = skb_vlan_tag_get_id(skb);
> +
> tuplehash = flow_offload_lookup(flow_table, &tuple);
> if (tuplehash == NULL)
> return NF_ACCEPT;
>
> dir = tuplehash->tuple.dir;
> flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
> - rt = (struct rtable *)flow->tuplehash[dir].tuple.dst.dst_cache;
> - outdev = rt->dst.dev;
> + if (family == NFPROTO_IPV4) {
> + rt = (struct rtable *)flow->tuplehash[dir].tuple.dst.dst_cache;
> + outdev = rt->dst.dev;
> + } else {
> + vlan_tag = flow->tuplehash[dir].tuple.dst.dst_port.dst_vlan_tag;
> + outdev = flow->tuplehash[dir].tuple.dst.dst_port.dev;
> + }
>
> if (unlikely(nf_flow_exceeds_mtu(skb, flow->tuplehash[dir].tuple.mtu)))
> return NF_ACCEPT;
> @@ -258,13 +273,25 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
> return NF_DROP;
>
> flow->timeout = (u32)jiffies + NF_FLOW_TIMEOUT;
> - iph = ip_hdr(skb);
> - ip_decrease_ttl(iph);
>
> skb->dev = outdev;
> - nexthop = rt_nexthop(rt, flow->tuplehash[!dir].tuple.src_v4.s_addr);
> - skb_dst_set_noref(skb, &rt->dst);
> - neigh_xmit(NEIGH_ARP_TABLE, outdev, &nexthop, skb);
> + if (family == NFPROTO_IPV4) {
> + iph = ip_hdr(skb);
> + ip_decrease_ttl(iph);
> +
> + nexthop = rt_nexthop(rt, flow->tuplehash[!dir].tuple.src_v4.s_addr);
> + skb_dst_set_noref(skb, &rt->dst);
> + neigh_xmit(NEIGH_ARP_TABLE, outdev, &nexthop, skb);
> + } else {
> + const struct net_bridge_port *p;
> +
> + if (vlan_tag && (p = br_port_get_rtnl_rcu(state->in)))
> + __vlan_hwaccel_put_tag(skb, p->br->vlan_proto, vlan_tag);
> + else
> + __vlan_hwaccel_clear_tag(skb);
> +
> + br_dev_queue_push_xmit(state->net, state->sk, skb);
> + }
Probably you can place common code into functions, the make a function
that uses these function to build the bridge and the inet datapath
into independent functions.
Instead of all these if() branches so often to reuse code, which makes
the code hard to follow.
> return NF_STOLEN;
> }
> diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> index cdb7c46..c88396a 100644
> --- a/net/netfilter/nft_flow_offload.c
> +++ b/net/netfilter/nft_flow_offload.c
> @@ -14,6 +14,8 @@
> #include <linux/netfilter/nf_conntrack_common.h>
> #include <net/netfilter/nf_flow_table.h>
>
> +#include "../bridge/br_private.h"
> +
> struct nft_flow_offload {
> struct nft_flowtable *flowtable;
> };
> @@ -49,6 +51,58 @@ static int nft_flow_route(const struct nft_pktinfo *pkt,
> return 0;
> }
>
> +static int nft_flow_forward(const struct nft_pktinfo *pkt,
> + const struct nf_conn *ct,
> + struct nf_flow_forward *forward,
> + enum ip_conntrack_dir dir)
> +{
> + struct net_bridge_vlan_group *vg;
> + const struct net_bridge_port *p;
> + u16 vid = 0;
> +
> + if (skb_vlan_tag_present(pkt->skb))
> + vid = skb_vlan_tag_get_id(pkt->skb);
> +
> + forward->tuple[dir].dst_port.dst_vlan_tag = vid;
> + forward->tuple[!dir].vlan_tag = vid;
> + forward->tuple[dir].dst_port.dev = dev_get_by_index(dev_net(nft_out(pkt)),
> + nft_out(pkt)->ifindex);
> + forward->tuple[!dir].dst_port.dev = dev_get_by_index(dev_net(nft_in(pkt)),
> + nft_in(pkt)->ifindex);
> +
> + rtnl_lock();
rtnl_lock() from the packet path?
> + p = br_port_get_rtnl_rcu(nft_out(pkt));
> + if (p) {
> + if (!br_opt_get(p->br, BROPT_VLAN_ENABLED))
> + goto out;
> +
> + if (!vid) {
> + vg = nbp_vlan_group_rcu(p);
> + vid = br_get_pvid(vg);
> + }
> +
> + if (vid) {
> + struct bridge_vlan_info info;
> +
> + if (br_vlan_get_info(nft_in(pkt), vid, &info) == 0 &&
> + info.flags & BRIDGE_VLAN_INFO_UNTAGGED)
> + vid = 0;
> + }
> + } else {
> + rtnl_unlock();
> + dev_put(forward->tuple[dir].dst_port.dev);
> + dev_put(forward->tuple[!dir].dst_port.dev);
> + return -ENOENT;
> + }
> +
> +out:
> + rtnl_unlock();
> + forward->tuple[!dir].dst_port.dst_vlan_tag = vid;
> + forward->tuple[dir].vlan_tag = vid;
> +
> + return 0;
> +}
> +
> static bool nft_flow_offload_skip(struct sk_buff *skb, int family)
> {
> if (skb_sec_path(skb))
> @@ -61,6 +115,15 @@ static bool nft_flow_offload_skip(struct sk_buff *skb, int family)
>
> if (unlikely(opt->optlen))
> return true;
> + } else if (family == NFPROTO_BRIDGE) {
> + const struct iphdr *iph;
> +
> + if (skb->protocol != htons(ETH_P_IP))
> + return true;
> +
> + iph = ip_hdr(skb);
> + if (iph->ihl > 5)
> + return true;
> }
>
> return false;
> @@ -76,11 +139,12 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
> struct nf_flow_data data;
> struct flow_offload *flow;
> enum ip_conntrack_dir dir;
> + int family = nft_pf(pkt);
> bool is_tcp = false;
> struct nf_conn *ct;
> int ret;
>
> - if (nft_flow_offload_skip(pkt->skb, nft_pf(pkt)))
> + if (nft_flow_offload_skip(pkt->skb, family))
> goto out;
>
> ct = nf_ct_get(pkt->skb, &ctinfo);
> @@ -108,8 +172,15 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
> goto out;
>
> dir = CTINFO2DIR(ctinfo);
> - if (nft_flow_route(pkt, ct, &data.route, dir) < 0)
> - goto err_flow_route;
> + if (family == NFPROTO_BRIDGE) {
> + data.type = FLOW_OFFLOAD_TYPE_BRIDGE;
> + if (nft_flow_forward(pkt, ct, &data.forward, dir) < 0)
> + goto err_flow_data;
> + } else {
> + data.type = FLOW_OFFLOAD_TYPE_INET;
> + if (nft_flow_route(pkt, ct, &data.route, dir) < 0)
> + goto err_flow_data;
> + }
>
> flow = flow_offload_alloc(ct, &data);
> if (!flow)
> @@ -124,14 +195,20 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
> if (ret < 0)
> goto err_flow_add;
>
> - dst_release(data.route.tuple[!dir].dst);
> + if (family != NFPROTO_BRIDGE)
> + dst_release(data.route.tuple[!dir].dst);
> return;
>
> err_flow_add:
> flow_offload_free(flow);
> err_flow_alloc:
> - dst_release(data.route.tuple[!dir].dst);
> -err_flow_route:
> + if (family == NFPROTO_BRIDGE) {
> + dev_put(data.forward.tuple[dir].dst_port.dev);
> + dev_put(data.forward.tuple[!dir].dst_port.dev);
> + } else {
> + dst_release(data.route.tuple[!dir].dst);
> + }
> +err_flow_data:
> clear_bit(IPS_OFFLOAD_BIT, &ct->status);
> out:
> regs->verdict.code = NFT_BREAK;
> --
> 1.8.3.1
>
^ permalink raw reply
* [PATCH net v2] net: make skb_dst_force return true when dst is refcounted
From: Florian Westphal @ 2019-06-26 18:40 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet, netfilter-devel, Florian Westphal
netfilter did not expect that skb_dst_force() can cause skb to lose its
dst entry.
I got a bug report with a skb->dst NULL dereference in netfilter
output path. The backtrace contains nf_reinject(), so the dst might have
been cleared when skb got queued to userspace.
Other users were fixed via
if (skb_dst(skb)) {
skb_dst_force(skb);
if (!skb_dst(skb))
goto handle_err;
}
But I think its preferable to make the 'dst might be cleared' part
of the function explicit.
In netfilter case, skb with a null dst is expected when queueing in
prerouting hook, so drop skb for the other hooks.
v2:
v1 of this patch returned true in case skb had no dst entry.
Eric said:
Say if we have two skb_dst_force() calls for some reason
on the same skb, only the first one will return false.
This now returns false even when skb had no dst, as per Erics
suggestion, so callers might need to check skb_dst() first before
skb_dst_force().
Signed-off-by: Florian Westphal <fw@strlen.de>
---
include/net/dst.h | 5 ++++-
net/netfilter/nf_queue.c | 6 +++++-
2 files changed, 9 insertions(+), 2 deletions(-)
Alternatively this could be routed via netfilter tree, let me
know your preference.
I've submitted a nfqueue testcase to nf-devel@ , it still passes after
this change.
diff --git a/include/net/dst.h b/include/net/dst.h
index 12b31c602cb0..f8206d3fed2f 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -302,8 +302,9 @@ static inline bool dst_hold_safe(struct dst_entry *dst)
* @skb: buffer
*
* If dst is not yet refcounted and not destroyed, grab a ref on it.
+ * Returns true if dst is refcounted.
*/
-static inline void skb_dst_force(struct sk_buff *skb)
+static inline bool skb_dst_force(struct sk_buff *skb)
{
if (skb_dst_is_noref(skb)) {
struct dst_entry *dst = skb_dst(skb);
@@ -314,6 +315,8 @@ static inline void skb_dst_force(struct sk_buff *skb)
skb->_skb_refdst = (unsigned long)dst;
}
+
+ return skb->_skb_refdst != 0UL;
}
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index b5b2be55ca82..2c440015ff0c 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -190,6 +190,11 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
goto err;
}
+ if (!skb_dst_force(skb) && state->hook != NF_INET_PRE_ROUTING) {
+ status = -ENETDOWN;
+ goto err;
+ }
+
*entry = (struct nf_queue_entry) {
.skb = skb,
.state = *state,
@@ -198,7 +203,6 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
};
nf_queue_entry_get_refs(entry);
- skb_dst_force(skb);
switch (entry->state.pf) {
case AF_INET:
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 2/3 nf-next] netfilter:nf_flow_table: Support bridge type flow offload
From: Florian Westphal @ 2019-06-26 18:38 UTC (permalink / raw)
To: wenxu; +Cc: pablo, fw, netfilter-devel, netdev
In-Reply-To: <1561545148-11978-2-git-send-email-wenxu@ucloud.cn>
wenxu@ucloud.cn <wenxu@ucloud.cn> wrote:
> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index 0016bb8..9af01ef 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> - neigh_xmit(NEIGH_ARP_TABLE, outdev, &nexthop, skb);
> + if (family == NFPROTO_IPV4) {
> + iph = ip_hdr(skb);
> + ip_decrease_ttl(iph);
> +
> + nexthop = rt_nexthop(rt, flow->tuplehash[!dir].tuple.src_v4.s_addr);
> + skb_dst_set_noref(skb, &rt->dst);
> + neigh_xmit(NEIGH_ARP_TABLE, outdev, &nexthop, skb);
> + } else {
> + const struct net_bridge_port *p;
> +
> + if (vlan_tag && (p = br_port_get_rtnl_rcu(state->in)))
> + __vlan_hwaccel_put_tag(skb, p->br->vlan_proto, vlan_tag);
> + else
> + __vlan_hwaccel_clear_tag(skb);
> +
> + br_dev_queue_push_xmit(state->net, state->sk, skb);
Won't that result in a module dep on bridge?
Whats the idea with this patch?
Do you see a performance improvement when bypassing bridge layer? If so,
how much?
I just wonder if its really cheaper than not using bridge conntrack in
the first place :-)
^ permalink raw reply
* Re: [PATCH bpf v4 2/2] xdp: fix hang while unregistering device bound to xdp socket
From: Jakub Kicinski @ 2019-06-26 18:34 UTC (permalink / raw)
To: Ilya Maximets
Cc: netdev, linux-kernel, bpf, xdp-newbies, David S. Miller,
Björn Töpel, Magnus Karlsson, Jonathan Lemon,
Daniel Borkmann
In-Reply-To: <20190626181515.1640-3-i.maximets@samsung.com>
On Wed, 26 Jun 2019 21:15:15 +0300, Ilya Maximets wrote:
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index 267b82a4cbcf..56729e74cbea 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -140,34 +140,38 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> return err;
> }
>
> -static void xdp_umem_clear_dev(struct xdp_umem *umem)
> +void xdp_umem_clear_dev(struct xdp_umem *umem)
> {
> + bool lock = rtnl_is_locked();
How do you know it's not just locked by someone else? You need to pass
the locked state in if this is called from different paths, some of
which already hold rtnl.
Preferably factor the code which needs the lock out into a separate
function like this:
void __function()
{
do();
the();
things();
under();
the();
lock();
}
void function()
{
rtnl_lock();
__function();
rtnl_unlock();
}
> struct netdev_bpf bpf;
> int err;
>
> + if (!lock)
> + rtnl_lock();
^ permalink raw reply
* Re: [PATCH 1/3 nf-next] netfilter:nf_flow_table: Refactor flow_offload_tuple to support more offload method
From: Pablo Neira Ayuso @ 2019-06-26 18:29 UTC (permalink / raw)
To: wenxu; +Cc: fw, netfilter-devel, netdev
In-Reply-To: <1561545148-11978-1-git-send-email-wenxu@ucloud.cn>
On Wed, Jun 26, 2019 at 06:32:26PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
Fix email subject, to:
netfilter: nf_flow_table: Refactor flow_offload_tuple to destination
> Add struct flow_offload_dst to support more offload method to replace
> dst_cache which only work for route offload.
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
> include/net/netfilter/nf_flow_table.h | 12 ++++++++++--
> net/netfilter/nf_flow_table_core.c | 22 +++++++++++-----------
> net/netfilter/nf_flow_table_ip.c | 4 ++--
> net/netfilter/nft_flow_offload.c | 10 +++++-----
> 4 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
> index d8c1879..968be64 100644
> --- a/include/net/netfilter/nf_flow_table.h
> +++ b/include/net/netfilter/nf_flow_table.h
> @@ -33,6 +33,10 @@ enum flow_offload_tuple_dir {
> FLOW_OFFLOAD_DIR_MAX = IP_CT_DIR_MAX
> };
>
> +struct flow_offload_dst {
> + struct dst_entry *dst_cache;
> +};
> +
> struct flow_offload_tuple {
> union {
> struct in_addr src_v4;
> @@ -55,7 +59,7 @@ struct flow_offload_tuple {
>
> u16 mtu;
>
> - struct dst_entry *dst_cache;
> + struct flow_offload_dst dst;
> };
>
> struct flow_offload_tuple_rhash {
> @@ -85,8 +89,12 @@ struct nf_flow_route {
> } tuple[FLOW_OFFLOAD_DIR_MAX];
> };
>
> +struct nf_flow_data {
Please, call this:
struct nf_flow_dst
instead.
> + struct nf_flow_route route;
> +};
> +
> struct flow_offload *flow_offload_alloc(struct nf_conn *ct,
> - struct nf_flow_route *route);
> + struct nf_flow_data *data);
> void flow_offload_free(struct flow_offload *flow);
>
> int flow_offload_add(struct nf_flowtable *flow_table, struct flow_offload *flow);
> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
> index e3d7972..125ce1c 100644
> --- a/net/netfilter/nf_flow_table_core.c
> +++ b/net/netfilter/nf_flow_table_core.c
> @@ -24,13 +24,13 @@ struct flow_offload_entry {
>
> static void
> flow_offload_fill_dir(struct flow_offload *flow, struct nf_conn *ct,
> - struct nf_flow_route *route,
> + struct nf_flow_data *data,
> enum flow_offload_tuple_dir dir)
> {
> struct flow_offload_tuple *ft = &flow->tuplehash[dir].tuple;
> struct nf_conntrack_tuple *ctt = &ct->tuplehash[dir].tuple;
> - struct dst_entry *other_dst = route->tuple[!dir].dst;
> - struct dst_entry *dst = route->tuple[dir].dst;
> + struct dst_entry *other_dst = date->route.tuple[!dir].dst;
> + struct dst_entry *dst = data->route.tuple[dir].dst;
>
> ft->dir = dir;
>
> @@ -57,7 +57,7 @@ struct flow_offload_entry {
> }
>
> struct flow_offload *
> -flow_offload_alloc(struct nf_conn *ct, struct nf_flow_route *route)
> +flow_offload_alloc(struct nf_conn *ct, struct nf_flow_data *data)
> {
> struct flow_offload_entry *entry;
> struct flow_offload *flow;
> @@ -72,16 +72,16 @@ struct flow_offload *
>
> flow = &entry->flow;
>
> - if (!dst_hold_safe(route->tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
> + if (!dst_hold_safe(data->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
> goto err_dst_cache_original;
>
> - if (!dst_hold_safe(route->tuple[FLOW_OFFLOAD_DIR_REPLY].dst))
> + if (!dst_hold_safe(data->route.tuple[FLOW_OFFLOAD_DIR_REPLY].dst))
> goto err_dst_cache_reply;
>
> entry->ct = ct;
>
> - flow_offload_fill_dir(flow, ct, route, FLOW_OFFLOAD_DIR_ORIGINAL);
> - flow_offload_fill_dir(flow, ct, route, FLOW_OFFLOAD_DIR_REPLY);
> + flow_offload_fill_dir(flow, ct, data, FLOW_OFFLOAD_DIR_ORIGINAL);
> + flow_offload_fill_dir(flow, ct, data, FLOW_OFFLOAD_DIR_REPLY);
>
> if (ct->status & IPS_SRC_NAT)
> flow->flags |= FLOW_OFFLOAD_SNAT;
> @@ -91,7 +91,7 @@ struct flow_offload *
> return flow;
>
> err_dst_cache_reply:
> - dst_release(route->tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
> + dst_release(data->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
> err_dst_cache_original:
> kfree(entry);
> err_ct_refcnt:
> @@ -139,8 +139,8 @@ void flow_offload_free(struct flow_offload *flow)
> {
> struct flow_offload_entry *e;
>
> - dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst_cache);
> - dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst_cache);
> + dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_cache);
> + dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_cache);
> e = container_of(flow, struct flow_offload_entry, flow);
> if (flow->flags & FLOW_OFFLOAD_DYING)
> nf_ct_delete(e->ct, 0, 0);
> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index 2413174..0016bb8 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -241,7 +241,7 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
>
> dir = tuplehash->tuple.dir;
> flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
> - rt = (struct rtable *)flow->tuplehash[dir].tuple.dst_cache;
> + rt = (struct rtable *)flow->tuplehash[dir].tuple.dst.dst_cache;
> outdev = rt->dst.dev;
>
> if (unlikely(nf_flow_exceeds_mtu(skb, flow->tuplehash[dir].tuple.mtu)))
> @@ -457,7 +457,7 @@ static int nf_flow_tuple_ipv6(struct sk_buff *skb, const struct net_device *dev,
>
> dir = tuplehash->tuple.dir;
> flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
> - rt = (struct rt6_info *)flow->tuplehash[dir].tuple.dst_cache;
> + rt = (struct rt6_info *)flow->tuplehash[dir].tuple.dst.dst_cache;
> outdev = rt->dst.dev;
>
> if (unlikely(nf_flow_exceeds_mtu(skb, flow->tuplehash[dir].tuple.mtu)))
> diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> index aa5f571..cdb7c46 100644
> --- a/net/netfilter/nft_flow_offload.c
> +++ b/net/netfilter/nft_flow_offload.c
> @@ -73,7 +73,7 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
> struct nft_flow_offload *priv = nft_expr_priv(expr);
> struct nf_flowtable *flowtable = &priv->flowtable->data;
> enum ip_conntrack_info ctinfo;
> - struct nf_flow_route route;
> + struct nf_flow_data data;
Please, reverse xmas tree for variable definition, from longest line
to shortest one.
> struct flow_offload *flow;
> enum ip_conntrack_dir dir;
> bool is_tcp = false;
> @@ -108,10 +108,10 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
> goto out;
>
> dir = CTINFO2DIR(ctinfo);
> - if (nft_flow_route(pkt, ct, &route, dir) < 0)
> + if (nft_flow_route(pkt, ct, &data.route, dir) < 0)
> goto err_flow_route;
>
> - flow = flow_offload_alloc(ct, &route);
> + flow = flow_offload_alloc(ct, &data);
> if (!flow)
> goto err_flow_alloc;
>
> @@ -124,13 +124,13 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
> if (ret < 0)
> goto err_flow_add;
>
> - dst_release(route.tuple[!dir].dst);
> + dst_release(data.route.tuple[!dir].dst);
> return;
>
> err_flow_add:
> flow_offload_free(flow);
> err_flow_alloc:
> - dst_release(route.tuple[!dir].dst);
> + dst_release(data.route.tuple[!dir].dst);
> err_flow_route:
> clear_bit(IPS_OFFLOAD_BIT, &ct->status);
> out:
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v2 bpf-next 04/11] libbpf: refactor map initialization
From: Andrii Nakryiko @ 2019-06-26 18:28 UTC (permalink / raw)
To: Matt Hart
Cc: Andrii Nakryiko, linux-perf-users, Alexei Starovoitov,
Daniel Borkmann, Networking, bpf, Kernel Team
In-Reply-To: <CAH+k93H5ddp7cMBGtyn9DNPvv1MymGCsShJ1-3tezLPUnxt6vw@mail.gmail.com>
On Wed, Jun 26, 2019 at 7:48 AM Matt Hart <matthew.hart@linaro.org> wrote:
>
> Hi all,
>
> I noticed perf fails to build for armv7 on linux next, due to this
> compile error:
> $ make -C tools/perf ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf-
>
> CC libbpf_probes.o
> In file included from libbpf.c:27:
> libbpf.c: In function ‘bpf_object__add_map’:
> /home/matt/git/linux-next/tools/include/linux/kernel.h:45:17: error:
> comparison of distinct pointer types lacks a cast [-Werror]
> (void) (&_max1 == &_max2); \
> ^~
> libbpf.c:776:12: note: in expansion of macro ‘max’
> new_cap = max(4ul, obj->maps_cap * 3 / 2);
> ^~~
>
> So I bisected it and came down to this patch.
> Commit bf82927125dd25003d76ed5541da704df21de57a
>
> Full verbose bisect script https://hastebin.com/odoyujofav.coffeescript
>
> Is this a case that perf code needs updating to match the change, or
> is the change broken?
Hi Matt,
Thanks for reporting. This issue was already fixed in
https://patchwork.ozlabs.org/patch/1122673/, so just pull latest
bpf-next.
>
>
>
> On Tue, 25 Jun 2019 at 16:53, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > User and global data maps initialization has gotten pretty complicated
> > and unnecessarily convoluted. This patch splits out the logic for global
> > data map and user-defined map initialization. It also removes the
> > restriction of pre-calculating how many maps will be initialized,
> > instead allowing to keep adding new maps as they are discovered, which
> > will be used later for BTF-defined map definitions.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/libbpf.c | 247 ++++++++++++++++++++++-------------------
> > 1 file changed, 133 insertions(+), 114 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 7ee44d8877c5..88609dca4f7d 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -234,6 +234,7 @@ struct bpf_object {
> > size_t nr_programs;
> > struct bpf_map *maps;
> > size_t nr_maps;
> > + size_t maps_cap;
> > struct bpf_secdata sections;
> >
> > bool loaded;
> > @@ -763,21 +764,51 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
> > return -ENOENT;
> > }
> >
> > -static bool bpf_object__has_maps(const struct bpf_object *obj)
> > +static struct bpf_map *bpf_object__add_map(struct bpf_object *obj)
> > {
> > - return obj->efile.maps_shndx >= 0 ||
> > - obj->efile.data_shndx >= 0 ||
> > - obj->efile.rodata_shndx >= 0 ||
> > - obj->efile.bss_shndx >= 0;
> > + struct bpf_map *new_maps;
> > + size_t new_cap;
> > + int i;
> > +
> > + if (obj->nr_maps < obj->maps_cap)
> > + return &obj->maps[obj->nr_maps++];
> > +
> > + new_cap = max(4ul, obj->maps_cap * 3 / 2);
> > + new_maps = realloc(obj->maps, new_cap * sizeof(*obj->maps));
> > + if (!new_maps) {
> > + pr_warning("alloc maps for object failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + obj->maps_cap = new_cap;
> > + obj->maps = new_maps;
> > +
> > + /* zero out new maps */
> > + memset(obj->maps + obj->nr_maps, 0,
> > + (obj->maps_cap - obj->nr_maps) * sizeof(*obj->maps));
> > + /*
> > + * fill all fd with -1 so won't close incorrect fd (fd=0 is stdin)
> > + * when failure (zclose won't close negative fd)).
> > + */
> > + for (i = obj->nr_maps; i < obj->maps_cap; i++) {
> > + obj->maps[i].fd = -1;
> > + obj->maps[i].inner_map_fd = -1;
> > + }
> > +
> > + return &obj->maps[obj->nr_maps++];
> > }
> >
> > static int
> > -bpf_object__init_internal_map(struct bpf_object *obj, struct bpf_map *map,
> > - enum libbpf_map_type type, Elf_Data *data,
> > - void **data_buff)
> > +bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
> > + Elf_Data *data, void **data_buff)
> > {
> > - struct bpf_map_def *def = &map->def;
> > char map_name[BPF_OBJ_NAME_LEN];
> > + struct bpf_map_def *def;
> > + struct bpf_map *map;
> > +
> > + map = bpf_object__add_map(obj);
> > + if (IS_ERR(map))
> > + return PTR_ERR(map);
> >
> > map->libbpf_type = type;
> > map->offset = ~(typeof(map->offset))0;
> > @@ -789,6 +820,7 @@ bpf_object__init_internal_map(struct bpf_object *obj, struct bpf_map *map,
> > return -ENOMEM;
> > }
> >
> > + def = &map->def;
> > def->type = BPF_MAP_TYPE_ARRAY;
> > def->key_size = sizeof(int);
> > def->value_size = data->d_size;
> > @@ -808,29 +840,58 @@ bpf_object__init_internal_map(struct bpf_object *obj, struct bpf_map *map,
> > return 0;
> > }
> >
> > -static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > +static int bpf_object__init_global_data_maps(struct bpf_object *obj)
> > +{
> > + int err;
> > +
> > + if (!obj->caps.global_data)
> > + return 0;
> > + /*
> > + * Populate obj->maps with libbpf internal maps.
> > + */
> > + if (obj->efile.data_shndx >= 0) {
> > + err = bpf_object__init_internal_map(obj, LIBBPF_MAP_DATA,
> > + obj->efile.data,
> > + &obj->sections.data);
> > + if (err)
> > + return err;
> > + }
> > + if (obj->efile.rodata_shndx >= 0) {
> > + err = bpf_object__init_internal_map(obj, LIBBPF_MAP_RODATA,
> > + obj->efile.rodata,
> > + &obj->sections.rodata);
> > + if (err)
> > + return err;
> > + }
> > + if (obj->efile.bss_shndx >= 0) {
> > + err = bpf_object__init_internal_map(obj, LIBBPF_MAP_BSS,
> > + obj->efile.bss, NULL);
> > + if (err)
> > + return err;
> > + }
> > + return 0;
> > +}
> > +
> > +static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
> > {
> > - int i, map_idx, map_def_sz = 0, nr_syms, nr_maps = 0, nr_maps_glob = 0;
> > - bool strict = !(flags & MAPS_RELAX_COMPAT);
> > Elf_Data *symbols = obj->efile.symbols;
> > + int i, map_def_sz = 0, nr_maps = 0, nr_syms;
> > Elf_Data *data = NULL;
> > - int ret = 0;
> > + Elf_Scn *scn;
> > +
> > + if (obj->efile.maps_shndx < 0)
> > + return 0;
> >
> > if (!symbols)
> > return -EINVAL;
> > - nr_syms = symbols->d_size / sizeof(GElf_Sym);
> > -
> > - if (obj->efile.maps_shndx >= 0) {
> > - Elf_Scn *scn = elf_getscn(obj->efile.elf,
> > - obj->efile.maps_shndx);
> >
> > - if (scn)
> > - data = elf_getdata(scn, NULL);
> > - if (!scn || !data) {
> > - pr_warning("failed to get Elf_Data from map section %d\n",
> > - obj->efile.maps_shndx);
> > - return -EINVAL;
> > - }
> > + scn = elf_getscn(obj->efile.elf, obj->efile.maps_shndx);
> > + if (scn)
> > + data = elf_getdata(scn, NULL);
> > + if (!scn || !data) {
> > + pr_warning("failed to get Elf_Data from map section %d\n",
> > + obj->efile.maps_shndx);
> > + return -EINVAL;
> > }
> >
> > /*
> > @@ -840,16 +901,8 @@ static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > *
> > * TODO: Detect array of map and report error.
> > */
> > - if (obj->caps.global_data) {
> > - if (obj->efile.data_shndx >= 0)
> > - nr_maps_glob++;
> > - if (obj->efile.rodata_shndx >= 0)
> > - nr_maps_glob++;
> > - if (obj->efile.bss_shndx >= 0)
> > - nr_maps_glob++;
> > - }
> > -
> > - for (i = 0; data && i < nr_syms; i++) {
> > + nr_syms = symbols->d_size / sizeof(GElf_Sym);
> > + for (i = 0; i < nr_syms; i++) {
> > GElf_Sym sym;
> >
> > if (!gelf_getsym(symbols, i, &sym))
> > @@ -858,79 +911,56 @@ static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > continue;
> > nr_maps++;
> > }
> > -
> > - if (!nr_maps && !nr_maps_glob)
> > - return 0;
> > -
> > /* Assume equally sized map definitions */
> > - if (data) {
> > - pr_debug("maps in %s: %d maps in %zd bytes\n", obj->path,
> > - nr_maps, data->d_size);
> > -
> > - map_def_sz = data->d_size / nr_maps;
> > - if (!data->d_size || (data->d_size % nr_maps) != 0) {
> > - pr_warning("unable to determine map definition size "
> > - "section %s, %d maps in %zd bytes\n",
> > - obj->path, nr_maps, data->d_size);
> > - return -EINVAL;
> > - }
> > - }
> > -
> > - nr_maps += nr_maps_glob;
> > - obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
> > - if (!obj->maps) {
> > - pr_warning("alloc maps for object failed\n");
> > - return -ENOMEM;
> > - }
> > - obj->nr_maps = nr_maps;
> > -
> > - for (i = 0; i < nr_maps; i++) {
> > - /*
> > - * fill all fd with -1 so won't close incorrect
> > - * fd (fd=0 is stdin) when failure (zclose won't close
> > - * negative fd)).
> > - */
> > - obj->maps[i].fd = -1;
> > - obj->maps[i].inner_map_fd = -1;
> > + pr_debug("maps in %s: %d maps in %zd bytes\n",
> > + obj->path, nr_maps, data->d_size);
> > +
> > + map_def_sz = data->d_size / nr_maps;
> > + if (!data->d_size || (data->d_size % nr_maps) != 0) {
> > + pr_warning("unable to determine map definition size "
> > + "section %s, %d maps in %zd bytes\n",
> > + obj->path, nr_maps, data->d_size);
> > + return -EINVAL;
> > }
> >
> > - /*
> > - * Fill obj->maps using data in "maps" section.
> > - */
> > - for (i = 0, map_idx = 0; data && i < nr_syms; i++) {
> > + /* Fill obj->maps using data in "maps" section. */
> > + for (i = 0; i < nr_syms; i++) {
> > GElf_Sym sym;
> > const char *map_name;
> > struct bpf_map_def *def;
> > + struct bpf_map *map;
> >
> > if (!gelf_getsym(symbols, i, &sym))
> > continue;
> > if (sym.st_shndx != obj->efile.maps_shndx)
> > continue;
> >
> > - map_name = elf_strptr(obj->efile.elf,
> > - obj->efile.strtabidx,
> > + map = bpf_object__add_map(obj);
> > + if (IS_ERR(map))
> > + return PTR_ERR(map);
> > +
> > + map_name = elf_strptr(obj->efile.elf, obj->efile.strtabidx,
> > sym.st_name);
> > if (!map_name) {
> > pr_warning("failed to get map #%d name sym string for obj %s\n",
> > - map_idx, obj->path);
> > + i, obj->path);
> > return -LIBBPF_ERRNO__FORMAT;
> > }
> >
> > - obj->maps[map_idx].libbpf_type = LIBBPF_MAP_UNSPEC;
> > - obj->maps[map_idx].offset = sym.st_value;
> > + map->libbpf_type = LIBBPF_MAP_UNSPEC;
> > + map->offset = sym.st_value;
> > if (sym.st_value + map_def_sz > data->d_size) {
> > pr_warning("corrupted maps section in %s: last map \"%s\" too small\n",
> > obj->path, map_name);
> > return -EINVAL;
> > }
> >
> > - obj->maps[map_idx].name = strdup(map_name);
> > - if (!obj->maps[map_idx].name) {
> > + map->name = strdup(map_name);
> > + if (!map->name) {
> > pr_warning("failed to alloc map name\n");
> > return -ENOMEM;
> > }
> > - pr_debug("map %d is \"%s\"\n", map_idx,
> > - obj->maps[map_idx].name);
> > + pr_debug("map %d is \"%s\"\n", i, map->name);
> > def = (struct bpf_map_def *)(data->d_buf + sym.st_value);
> > /*
> > * If the definition of the map in the object file fits in
> > @@ -939,7 +969,7 @@ static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > * calloc above.
> > */
> > if (map_def_sz <= sizeof(struct bpf_map_def)) {
> > - memcpy(&obj->maps[map_idx].def, def, map_def_sz);
> > + memcpy(&map->def, def, map_def_sz);
> > } else {
> > /*
> > * Here the map structure being read is bigger than what
> > @@ -959,37 +989,30 @@ static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > return -EINVAL;
> > }
> > }
> > - memcpy(&obj->maps[map_idx].def, def,
> > - sizeof(struct bpf_map_def));
> > + memcpy(&map->def, def, sizeof(struct bpf_map_def));
> > }
> > - map_idx++;
> > }
> > + return 0;
> > +}
> >
> > - if (!obj->caps.global_data)
> > - goto finalize;
> > +static int bpf_object__init_maps(struct bpf_object *obj, int flags)
> > +{
> > + bool strict = !(flags & MAPS_RELAX_COMPAT);
> > + int err;
> >
> > - /*
> > - * Populate rest of obj->maps with libbpf internal maps.
> > - */
> > - if (obj->efile.data_shndx >= 0)
> > - ret = bpf_object__init_internal_map(obj, &obj->maps[map_idx++],
> > - LIBBPF_MAP_DATA,
> > - obj->efile.data,
> > - &obj->sections.data);
> > - if (!ret && obj->efile.rodata_shndx >= 0)
> > - ret = bpf_object__init_internal_map(obj, &obj->maps[map_idx++],
> > - LIBBPF_MAP_RODATA,
> > - obj->efile.rodata,
> > - &obj->sections.rodata);
> > - if (!ret && obj->efile.bss_shndx >= 0)
> > - ret = bpf_object__init_internal_map(obj, &obj->maps[map_idx++],
> > - LIBBPF_MAP_BSS,
> > - obj->efile.bss, NULL);
> > -finalize:
> > - if (!ret)
> > + err = bpf_object__init_user_maps(obj, strict);
> > + if (err)
> > + return err;
> > +
> > + err = bpf_object__init_global_data_maps(obj);
> > + if (err)
> > + return err;
> > +
> > + if (obj->nr_maps) {
> > qsort(obj->maps, obj->nr_maps, sizeof(obj->maps[0]),
> > compare_bpf_map);
> > - return ret;
> > + }
> > + return 0;
> > }
> >
> > static bool section_have_execinstr(struct bpf_object *obj, int idx)
> > @@ -1262,14 +1285,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
> > return -LIBBPF_ERRNO__FORMAT;
> > }
> > err = bpf_object__load_btf(obj, btf_data, btf_ext_data);
> > - if (err)
> > - return err;
> > - if (bpf_object__has_maps(obj)) {
> > + if (!err)
> > err = bpf_object__init_maps(obj, flags);
> > - if (err)
> > - return err;
> > - }
> > - err = bpf_object__init_prog_names(obj);
> > + if (!err)
> > + err = bpf_object__init_prog_names(obj);
> > return err;
> > }
> >
> > --
> > 2.17.1
> >
^ permalink raw reply
* [PATCH bpf v4 1/2] xdp: hold device for umem regardless of zero-copy mode
From: Ilya Maximets @ 2019-06-26 18:15 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, bpf, xdp-newbies, David S. Miller,
Björn Töpel, Magnus Karlsson, Jonathan Lemon,
Jakub Kicinski, Daniel Borkmann, Ilya Maximets
In-Reply-To: <20190626181515.1640-1-i.maximets@samsung.com>
Device pointer stored in umem regardless of zero-copy mode,
so we heed to hold the device in all cases.
Fixes: c9b47cc1fabc ("xsk: fix bug when trying to use both copy and zero-copy on one queue id")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
net/xdp/xdp_umem.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 9c6de4f114f8..267b82a4cbcf 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -105,6 +105,9 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
umem->dev = dev;
umem->queue_id = queue_id;
+
+ dev_hold(dev);
+
if (force_copy)
/* For copy-mode, we are done. */
goto out_rtnl_unlock;
@@ -124,7 +127,6 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
goto err_unreg_umem;
rtnl_unlock();
- dev_hold(dev);
umem->zc = true;
return 0;
@@ -163,10 +165,9 @@ static void xdp_umem_clear_dev(struct xdp_umem *umem)
xdp_clear_umem_at_qid(umem->dev, umem->queue_id);
rtnl_unlock();
- if (umem->zc) {
- dev_put(umem->dev);
- umem->zc = false;
- }
+ dev_put(umem->dev);
+ umem->dev = NULL;
+ umem->zc = false;
}
static void xdp_umem_unpin_pages(struct xdp_umem *umem)
--
2.17.1
^ permalink raw reply related
* [PATCH bpf v4 2/2] xdp: fix hang while unregistering device bound to xdp socket
From: Ilya Maximets @ 2019-06-26 18:15 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, bpf, xdp-newbies, David S. Miller,
Björn Töpel, Magnus Karlsson, Jonathan Lemon,
Jakub Kicinski, Daniel Borkmann, Ilya Maximets
In-Reply-To: <20190626181515.1640-1-i.maximets@samsung.com>
Device that bound to XDP socket will not have zero refcount until the
userspace application will not close it. This leads to hang inside
'netdev_wait_allrefs()' if device unregistering requested:
# ip link del p1
< hang on recvmsg on netlink socket >
# ps -x | grep ip
5126 pts/0 D+ 0:00 ip link del p1
# journalctl -b
Jun 05 07:19:16 kernel:
unregister_netdevice: waiting for p1 to become free. Usage count = 1
Jun 05 07:19:27 kernel:
unregister_netdevice: waiting for p1 to become free. Usage count = 1
...
Fix that by implementing NETDEV_UNREGISTER event notification handler
to properly clean up all the resources and unref device.
This should also allow socket killing via ss(8) utility.
Fixes: 965a99098443 ("xsk: add support for bind for Rx")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
include/net/xdp_sock.h | 5 +++
net/xdp/xdp_umem.c | 16 +++++---
net/xdp/xdp_umem.h | 1 +
net/xdp/xsk.c | 88 ++++++++++++++++++++++++++++++++++++------
4 files changed, 93 insertions(+), 17 deletions(-)
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index d074b6d60f8a..82d153a637c7 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -61,6 +61,11 @@ struct xdp_sock {
struct xsk_queue *tx ____cacheline_aligned_in_smp;
struct list_head list;
bool zc;
+ enum {
+ XSK_UNINITIALIZED = 0,
+ XSK_BINDED,
+ XSK_UNBINDED,
+ } state;
/* Protects multiple processes in the control path */
struct mutex mutex;
/* Mutual exclusion of NAPI TX thread and sendmsg error paths
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 267b82a4cbcf..56729e74cbea 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -140,34 +140,38 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
return err;
}
-static void xdp_umem_clear_dev(struct xdp_umem *umem)
+void xdp_umem_clear_dev(struct xdp_umem *umem)
{
+ bool lock = rtnl_is_locked();
struct netdev_bpf bpf;
int err;
+ if (!lock)
+ rtnl_lock();
+
if (!umem->dev)
- return;
+ goto out_unlock;
if (umem->zc) {
bpf.command = XDP_SETUP_XSK_UMEM;
bpf.xsk.umem = NULL;
bpf.xsk.queue_id = umem->queue_id;
- rtnl_lock();
err = umem->dev->netdev_ops->ndo_bpf(umem->dev, &bpf);
- rtnl_unlock();
if (err)
WARN(1, "failed to disable umem!\n");
}
- rtnl_lock();
xdp_clear_umem_at_qid(umem->dev, umem->queue_id);
- rtnl_unlock();
dev_put(umem->dev);
umem->dev = NULL;
umem->zc = false;
+
+out_unlock:
+ if (!lock)
+ rtnl_unlock();
}
static void xdp_umem_unpin_pages(struct xdp_umem *umem)
diff --git a/net/xdp/xdp_umem.h b/net/xdp/xdp_umem.h
index 27603227601b..a63a9fb251f5 100644
--- a/net/xdp/xdp_umem.h
+++ b/net/xdp/xdp_umem.h
@@ -10,6 +10,7 @@
int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
u16 queue_id, u16 flags);
+void xdp_umem_clear_dev(struct xdp_umem *umem);
bool xdp_umem_validate_queues(struct xdp_umem *umem);
void xdp_get_umem(struct xdp_umem *umem);
void xdp_put_umem(struct xdp_umem *umem);
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index a14e8864e4fa..883dfd3cdc49 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -335,6 +335,22 @@ static int xsk_init_queue(u32 entries, struct xsk_queue **queue,
return 0;
}
+static void xsk_unbind_dev(struct xdp_sock *xs)
+{
+ struct net_device *dev = xs->dev;
+
+ if (!dev || xs->state != XSK_BINDED)
+ return;
+
+ xs->state = XSK_UNBINDED;
+
+ /* Wait for driver to stop using the xdp socket. */
+ xdp_del_sk_umem(xs->umem, xs);
+ xs->dev = NULL;
+ synchronize_net();
+ dev_put(dev);
+}
+
static int xsk_release(struct socket *sock)
{
struct sock *sk = sock->sk;
@@ -354,15 +370,7 @@ static int xsk_release(struct socket *sock)
sock_prot_inuse_add(net, sk->sk_prot, -1);
local_bh_enable();
- if (xs->dev) {
- struct net_device *dev = xs->dev;
-
- /* Wait for driver to stop using the xdp socket. */
- xdp_del_sk_umem(xs->umem, xs);
- xs->dev = NULL;
- synchronize_net();
- dev_put(dev);
- }
+ xsk_unbind_dev(xs);
xskq_destroy(xs->rx);
xskq_destroy(xs->tx);
@@ -412,7 +420,7 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
return -EINVAL;
mutex_lock(&xs->mutex);
- if (xs->dev) {
+ if (xs->state != XSK_UNINITIALIZED) {
err = -EBUSY;
goto out_release;
}
@@ -492,6 +500,8 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
out_unlock:
if (err)
dev_put(dev);
+ else
+ xs->state = XSK_BINDED;
out_release:
mutex_unlock(&xs->mutex);
return err;
@@ -520,6 +530,10 @@ static int xsk_setsockopt(struct socket *sock, int level, int optname,
return -EFAULT;
mutex_lock(&xs->mutex);
+ if (xs->state != XSK_UNINITIALIZED) {
+ mutex_unlock(&xs->mutex);
+ return -EBUSY;
+ }
q = (optname == XDP_TX_RING) ? &xs->tx : &xs->rx;
err = xsk_init_queue(entries, q, false);
mutex_unlock(&xs->mutex);
@@ -534,7 +548,7 @@ static int xsk_setsockopt(struct socket *sock, int level, int optname,
return -EFAULT;
mutex_lock(&xs->mutex);
- if (xs->umem) {
+ if (xs->state != XSK_UNINITIALIZED || xs->umem) {
mutex_unlock(&xs->mutex);
return -EBUSY;
}
@@ -561,6 +575,10 @@ static int xsk_setsockopt(struct socket *sock, int level, int optname,
return -EFAULT;
mutex_lock(&xs->mutex);
+ if (xs->state != XSK_UNINITIALIZED) {
+ mutex_unlock(&xs->mutex);
+ return -EBUSY;
+ }
if (!xs->umem) {
mutex_unlock(&xs->mutex);
return -EINVAL;
@@ -662,6 +680,9 @@ static int xsk_mmap(struct file *file, struct socket *sock,
unsigned long pfn;
struct page *qpg;
+ if (xs->state != XSK_UNINITIALIZED)
+ return -EBUSY;
+
if (offset == XDP_PGOFF_RX_RING) {
q = READ_ONCE(xs->rx);
} else if (offset == XDP_PGOFF_TX_RING) {
@@ -693,6 +715,38 @@ static int xsk_mmap(struct file *file, struct socket *sock,
size, vma->vm_page_prot);
}
+static int xsk_notifier(struct notifier_block *this,
+ unsigned long msg, void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+ struct net *net = dev_net(dev);
+ struct sock *sk;
+
+ switch (msg) {
+ case NETDEV_UNREGISTER:
+ mutex_lock(&net->xdp.lock);
+ sk_for_each(sk, &net->xdp.list) {
+ struct xdp_sock *xs = xdp_sk(sk);
+
+ mutex_lock(&xs->mutex);
+ if (xs->dev == dev) {
+ sk->sk_err = ENETDOWN;
+ if (!sock_flag(sk, SOCK_DEAD))
+ sk->sk_error_report(sk);
+
+ xsk_unbind_dev(xs);
+
+ /* Clear device references in umem. */
+ xdp_umem_clear_dev(xs->umem);
+ }
+ mutex_unlock(&xs->mutex);
+ }
+ mutex_unlock(&net->xdp.lock);
+ break;
+ }
+ return NOTIFY_DONE;
+}
+
static struct proto xsk_proto = {
.name = "XDP",
.owner = THIS_MODULE,
@@ -764,6 +818,7 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
sock_set_flag(sk, SOCK_RCU_FREE);
xs = xdp_sk(sk);
+ xs->state = XSK_UNINITIALIZED;
mutex_init(&xs->mutex);
spin_lock_init(&xs->tx_completion_lock);
@@ -784,6 +839,10 @@ static const struct net_proto_family xsk_family_ops = {
.owner = THIS_MODULE,
};
+static struct notifier_block xsk_netdev_notifier = {
+ .notifier_call = xsk_notifier,
+};
+
static int __net_init xsk_net_init(struct net *net)
{
mutex_init(&net->xdp.lock);
@@ -816,8 +875,15 @@ static int __init xsk_init(void)
err = register_pernet_subsys(&xsk_net_ops);
if (err)
goto out_sk;
+
+ err = register_netdevice_notifier(&xsk_netdev_notifier);
+ if (err)
+ goto out_pernet;
+
return 0;
+out_pernet:
+ unregister_pernet_subsys(&xsk_net_ops);
out_sk:
sock_unregister(PF_XDP);
out_proto:
--
2.17.1
^ permalink raw reply related
* [PATCH bpf v4 0/2] xdp: fix hang while unregistering device bound to xdp socket
From: Ilya Maximets @ 2019-06-26 18:15 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, bpf, xdp-newbies, David S. Miller,
Björn Töpel, Magnus Karlsson, Jonathan Lemon,
Jakub Kicinski, Daniel Borkmann, Ilya Maximets
In-Reply-To: <CGME20190626181520eucas1p1817d31d958b8755600f0745e92edef5a@eucas1p1.samsung.com>
Version 4:
* 'xdp_umem_clear_dev' exposed to be used while unregistering.
* Added XDP socket state to track if resources already unbinded.
* Splitted in two fixes.
Version 3:
* Declaration lines ordered from longest to shortest.
* Checking of event type moved to the top to avoid unnecessary
locking.
Version 2:
* Completely re-implemented using netdev event handler.
Ilya Maximets (2):
xdp: hold device for umem regardless of zero-copy mode
xdp: fix hang while unregistering device bound to xdp socket
include/net/xdp_sock.h | 5 +++
net/xdp/xdp_umem.c | 27 +++++++------
net/xdp/xdp_umem.h | 1 +
net/xdp/xsk.c | 88 ++++++++++++++++++++++++++++++++++++------
4 files changed, 99 insertions(+), 22 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: WWAN Controller Framework (was IPA [PATCH v2 00/17])
From: Johannes Berg @ 2019-06-26 17:58 UTC (permalink / raw)
To: Alex Elder, davem, arnd, bjorn.andersson, ilias.apalodimas,
Dan Williams
Cc: evgreen, benchan, ejcaruso, cpratapa, syadagir, subashab,
abhishek.esse, netdev, devicetree, linux-kernel, linux-soc,
linux-arm-kernel, linux-arm-msm
In-Reply-To: <25bb0936-686c-101b-c5a4-474ed37536aa@linaro.org>
On Wed, 2019-06-26 at 08:40 -0500, Alex Elder wrote:
> > I think here we need to be more careful. I don't know how you want to
> > call it, but we actually have multiple levels of control here.
>
> I completely agree with you. From what I understand there exists
> a control channel (or even more than one?) that serves a very
> specific purpose in modem management. The main reason I mention
> the WWAN control function is that someone (maybe you) indicated
> that a control channel automatically gets created.
It may or may not, right. I just bought a cheap used USB modem, and it
just comes with two USB TTY channels, so I guess for data it does PPP on
top of that. But those channels are created automatically once you
connect the device to the system.
OTOH, for something like the Intel modem, we might well decide not to
create *any* channels on driver load, since you have the option of using
AT commands or MBIM (but I believe both at the same time wouldn't really
make sense, if even allowed).
> > This ... depends a bit on how you exactly define a physical channel
> > here. Is that, to you, the PCIe/USB link? In that case, yes, obviously
> > you have only one physical channel for each WWAN unit.
>
> I think that was what I was trying to capture. There exists
> one or more "physical" communication paths between the AP
> and WWAN unit/modem. And while one path *could* carry just
> one type of traffic, it could also carry multiple logical
> channels of traffic by multiplexing.
Right.
(What I wasn't aware is that QMI is actually a different physical path.
I thought it was just a protocol multiplexed on top of the same IPA
physical path).
> I don't think I have any argument with this. I'm going to try to
> put together something that goes beyond what I wrote in this message,
> to try to capture what I think we agree on in a sort of loose design
> document.
Awesome, thanks a lot!
johannes
^ permalink raw reply
* Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver
From: Johannes Berg @ 2019-06-26 17:55 UTC (permalink / raw)
To: Alex Elder, Dan Williams, Arnd Bergmann
Cc: Subash Abhinov Kasiviswanathan, abhishek.esse, Ben Chan,
Bjorn Andersson, cpratapa, David Miller, DTML, Eric Caruso,
evgreen, Ilias Apalodimas, Linux ARM, linux-arm-msm,
Linux Kernel Mailing List, linux-soc, Networking, syadagir
In-Reply-To: <0f5c0332-6894-2fdd-fd25-7af9a21b445b@linaro.org>
On Wed, 2019-06-26 at 08:36 -0500, Alex Elder wrote:
>
> We need to identify the existence of a WWAN device (which is I
> guess--typically? always?--a modem). Perhaps that can be
> discovered in some cases but I think it means a node described
> by Device Tree.
Yeah, perhaps that's something you could do. I'm not sure though. For
one, for USB devices, obviously it isn't :-) And even for IPA you might
want to support existing DTs I guess.
> So you're saying you have a single Ethernet driver, and it can
> drive an Ethernet device connected to a WWAN, or not connected
> to a WWAN, without any changes. The only distinction is that
> if the device is part of a WWAN it needs to register with the
> WWAN framework. Is that right?
That's what I'm thinking, and I believe (mostly from discussions with
Dan) that this actually exists.
> > > So maybe:
> > > - Hardware probe detects a WWAN device
> > > - The drivers that detect the WWAN device register it with the
> > > WWAN core code.
> > > - A control channel is instantiated at/before the time the WWAN
> > > device is registered
> > > - Something in user space should manage the bring-up of any
> > > other things on the WWAN device thereafter
> >
> > But those things need to actually get connected first :-)
>
> What I meant is that the registering with the "WWAN core code"
> is what does that "connecting." The WWAN code has the information
> about what got registered. But as I said above, this WWAN device
> needs to be identified, and I think (at least for IPA) that will
> require something in Device Tree. That will "connect" them.
>
> Or I might be misunderstanding your point.
No, I think we're mostly agreeing, just thinking about different
scenarios. I think for IPA you don't really *need* anything in the DT
though - as soon as the IPA driver is loaded you know for sure you
actually have a modem there, and the IPA driver presumably loads based
on some existing probing (didn't look at it now).
Now, I don't know how the QMI channel to the modem is set up, so of
course you'd want a way of identifying that the two channels (IPA and
QMI) go to the same device and link them together in the WWAN framework.
> > If userspace actually had the ability to create (data) channels, then it
> > would have the ability to also remove them. Right now, this may or may
> > not be supported by the drivers that act together to form the interfaces
> > to a WWAN device.
>
> I think this (user space control) needs to be an option, but
> it doesn't have to be the only way.
Agree.
johannes
^ permalink raw reply
* Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver
From: Johannes Berg @ 2019-06-26 17:48 UTC (permalink / raw)
To: Arnd Bergmann, Alex Elder
Cc: Dan Williams, Subash Abhinov Kasiviswanathan, abhishek.esse,
Ben Chan, Bjorn Andersson, cpratapa, David Miller, DTML,
Eric Caruso, evgreen, Ilias Apalodimas, Linux ARM, linux-arm-msm,
Linux Kernel Mailing List, linux-soc, Networking, syadagir
In-Reply-To: <CAK8P3a3wHe_6ay8P+F9Vo=k19P5fifs6RWozxFF5nGYYjO_=Xw@mail.gmail.com>
On Wed, 2019-06-26 at 15:58 +0200, Arnd Bergmann wrote:
>
> > The IPA hardware is actually something that sits *between* the
> > AP and the modem. It implements one form of communication
> > pathway (IP data), but there are others (including QMI, which
> > presents a network-like interface but it's actually implemented
> > via clever use of shared memory and interrupts).
>
> Can you clarify how QMI fits in here? Do you mean one has to
> talk to both IPA and QMI to use the modem, or are these two
> alternative implementations for the same basic purpose?
I'm not going to comment on QMI specifically, because my understanding
might well be wrong, and any response to your question will likely
correct my understanding :-)
(Thus, you should probably also ignore everything I ever said about QMI)
> My previous understanding was that from the hardware perspective
> there is only one control interface, which is for IPA. Part of this
> is abstracted to user space with ioctl commands to the IPA driver,
> and then one must set up rmnet to match these by configuring
> an rmnet device over netlink messages from user space, but
> rmnet does not have a control protocol with the hardware.
Right so this is why I say it's confusing when we just talk about
"control interface" or "path".
I see multiple layers of control
* hardware control, which you mention here. This might be things like
"enable/disable aggregation on an rmnet channel" etc. I guess this
type of thing would have been implemented with ioctls? Not the
aggregation specifically, but things that affect how you set up the
hardware.
* modem control, which we conflate, but can be like AT commands or
MBIM. From the kernel driver POV, this is actually just another
channel it provides for userspace to talk to the modem.
johannes
^ permalink raw reply
* Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver
From: Johannes Berg @ 2019-06-26 17:45 UTC (permalink / raw)
To: Alex Elder, Arnd Bergmann
Cc: Dan Williams, Subash Abhinov Kasiviswanathan, abhishek.esse,
Ben Chan, Bjorn Andersson, cpratapa, David Miller, DTML,
Eric Caruso, evgreen, Ilias Apalodimas, Linux ARM, linux-arm-msm,
Linux Kernel Mailing List, linux-soc, Networking, syadagir
In-Reply-To: <edea19ef-f225-bdcd-f394-77e326d1d3ad@linaro.org>
On Wed, 2019-06-26 at 08:39 -0500, Alex Elder wrote:
> > However, to also reply to Alex: I don't know exactly how IPA works, but
> > for the Intel modem at least you can't fundamentally have two drivers
> > for different parts of the functionality, since it's just a single piece
> > of hardware and you need to allocate hardware resources from a common
> > pool etc. So you cannot split the driver into "Intel modem control
> > channel driver" and "Intel modem data channel driver". In fact, it's
> > just a single "struct device" on the PCIe bus that you can bind to, and
> > only one driver can bind at a time.
>
> Interesting. So a single modem driver needs to implement
> *all* of the features/functions? Like GPS or data log or
> whatever, all needs to share the same struct device?
> Or does what you're describing apply to a subset of the
> modem's functionality? Or something else?
Well, what is even the "implement the functions"? I mean, as kernel
drivers we're really just in the business of providing communication
channels to those functions. E.g. if you have a GNSS/GPS device, you
might just have another TTY channel with NMEA data coming out, or
something like that, right?
But from a kernel POV yes, I don't see how you could create multiple
function drivers for something behind the same PCIe device (unless it
actually appeared as multiple virtual functions or such, like the bigger
ethernet NICs, but it doesn't).
But this points out to me that I was actually not quite accurate when I
spoke about struct device before in the USB context with function
drivers, but I have to do some research before I can correct myself
correctly.
> > So, IOW, I'm not sure I see how you'd split that up. I guess you could
> > if you actually do something like the "rmnet" model, and I suppose
> > you're free to do that for IPA if you like, but I tend to think that's
> > actually a burden, not a win since you just get more complex code that
> > needs to interact with more pieces. A single driver for a single
> > hardware that knows about the few types of channels seems simpler to me.
> >
> > > - to answer Johannes question, my understanding is that the interface
> > > between kernel and firmware/hardware for IPA has a single 'struct
> > > device' that is used for both the data and the control channels,
> > > rather than having a data channel and an independent control device,
> > > so this falls into the same category as the Intel one (please correct
> > > me on that)
>
> I don't think that's quite right, but it might be partially
> right. There is a single device representing IPA, but the
> picture is a little more complicated.
>
> The IPA hardware is actually something that sits *between* the
> AP and the modem. It implements one form of communication
> pathway (IP data), but there are others (including QMI, which
> presents a network-like interface but it's actually implemented
> via clever use of shared memory and interrupts).
OK.
Well, I guess this then might eventually become a bit of a hybrid - you
eventually want one WWAN device to represent it all to userspace, but
might actually have multiple devices with different drivers (from the
kernel POV)?
But this is more like all the USB devices work. I just have to figure
out how to correctly tie them together - "struct device" may or may not
be right? I need to check how this functions.
I guess for something where you have DT (like you allude to elsewhere)
you could just capture all this in DT by having some phandle link or
something?
> What we're talking about here is WWAN/modem management more
> generally though. It *sounds* like the Intel modem is
> more like a single device, which requires a single driver,
> that seems to implement a bunch of distinct functions.
Yes.
> On this I'm not very knowledgeable but for Qualcomm there is
> user space code that is in charge of overall management of
> the modem. It implements what I think you're calling control
> functions, negotiating with the modem to allow new data channels
> to be created. Normally the IPA driver would provide information
> to user space about available resources, but would only make a
> communication pathway available when requested.
Right.
> > Are the control channels to IPA are actually also tunnelled over the
> > rmnet protocol? And even if they are, perhaps they have a different
> > hardware queue or so? That'd be the case for Intel - different hardware
> > queue, same (or at least similar) protocol spoken for the DMA hardware
> > itself, but different contents of the messages obviously.
>
> I want to be careful talking about "control" but for IPA it comes
> from user space. For the purpose of getting initial code upstream,
> all of that control functionality (which was IOCTL based) has been
> removed, and a fixed configuration is assumed.
But something that's ioctl based is just one form of "control" pathway.
I was thinking of the AT or MBIM commands "control" channel. And then
ioctls are likely something that terminates in the *driver*, right? I
mean, the driver wouldn't get an ioctl and actually talk AT commands to
the device ...
But yes, the various control planes are confusing, we need to
disentangle that. I tried over in the other email by layering where the
control terminates.
johannes
^ permalink raw reply
* Re: [PATCH net v2] ipv4: reset rt_iif for recirculated mcast/bcast out pkts
From: David Ahern @ 2019-06-26 17:28 UTC (permalink / raw)
To: Stephen Suryaputra, netdev
In-Reply-To: <20190626062116.4319-1-ssuryaextr@gmail.com>
On 6/26/19 12:21 AM, Stephen Suryaputra wrote:
> Multicast or broadcast egress packets have rt_iif set to the oif. These
> packets might be recirculated back as input and lookup to the raw
> sockets may fail because they are bound to the incoming interface
> (skb_iif). If rt_iif is not zero, during the lookup, inet_iif() function
> returns rt_iif instead of skb_iif. Hence, the lookup fails.
>
> v2: Make it non vrf specific (David Ahern). Reword the changelog to
> reflect it.
> Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
> ---
> include/net/route.h | 1 +
> net/ipv4/ip_output.c | 12 ++++++++++++
> net/ipv4/route.c | 33 +++++++++++++++++++++++++++++++++
> 3 files changed, 46 insertions(+)
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* INFO: rcu detected stall in ext4_write_checks
From: syzbot @ 2019-06-26 17:27 UTC (permalink / raw)
To: adilger.kernel, davem, eladr, idosch, jiri, john.stultz,
linux-ext4, linux-kernel, netdev, syzkaller-bugs, tglx, tytso
Hello,
syzbot found the following crash on:
HEAD commit: abf02e29 Merge tag 'pm-5.2-rc6' of git://git.kernel.org/pu..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1435aaf6a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=e5c77f8090a3b96b
dashboard link: https://syzkaller.appspot.com/bug?extid=4bfbbf28a2e50ab07368
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11234c41a00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15d7f026a00000
The bug was bisected to:
commit 0c81ea5db25986fb2a704105db454a790c59709c
Author: Elad Raz <eladr@mellanox.com>
Date: Fri Oct 28 19:35:58 2016 +0000
mlxsw: core: Add port type (Eth/IB) set API
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10393a89a00000
final crash: https://syzkaller.appspot.com/x/report.txt?x=12393a89a00000
console output: https://syzkaller.appspot.com/x/log.txt?x=14393a89a00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4bfbbf28a2e50ab07368@syzkaller.appspotmail.com
Fixes: 0c81ea5db259 ("mlxsw: core: Add port type (Eth/IB) set API")
rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
(detected by 0, t=10502 jiffies, g=8969, q=26)
rcu: All QSes seen, last rcu_preempt kthread activity 10503
(4295056736-4295046233), jiffies_till_next_fqs=1, root ->qsmask 0x0
syz-executor778 R running task 26464 9577 9576 0x00004000
Call Trace:
<IRQ>
sched_show_task kernel/sched/core.c:5286 [inline]
sched_show_task.cold+0x291/0x2fc kernel/sched/core.c:5261
print_other_cpu_stall kernel/rcu/tree_stall.h:410 [inline]
check_cpu_stall kernel/rcu/tree_stall.h:536 [inline]
rcu_pending kernel/rcu/tree.c:2625 [inline]
rcu_sched_clock_irq.cold+0xaaf/0xbfd kernel/rcu/tree.c:2161
update_process_times+0x32/0x80 kernel/time/timer.c:1639
tick_sched_handle+0xa2/0x190 kernel/time/tick-sched.c:167
tick_sched_timer+0x47/0x130 kernel/time/tick-sched.c:1298
__run_hrtimer kernel/time/hrtimer.c:1389 [inline]
__hrtimer_run_queues+0x33b/0xdd0 kernel/time/hrtimer.c:1451
hrtimer_interrupt+0x314/0x770 kernel/time/hrtimer.c:1509
local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1041 [inline]
smp_apic_timer_interrupt+0x111/0x550 arch/x86/kernel/apic/apic.c:1066
apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:806
</IRQ>
RIP: 0010:ext4_write_checks+0x1/0x260 fs/ext4/file.c:161
Code: 61 fa ff ff e8 e0 3c 53 ff 55 48 89 e5 41 54 49 89 fc e8 f2 0a 81 ff
4c 89 e7 31 f6 e8 98 f9 ff ff 41 5c 5d c3 0f 1f 40 00 55 <48> 89 e5 41 56
41 55 49 89 f5 41 54 53 48 89 fb e8 ca 0a 81 ff 48
RSP: 0018:ffff888093a97640 EFLAGS: 00000293 ORIG_RAX: ffffffffffffff13
RAX: ffff88809901c100 RBX: ffff888093a977d8 RCX: ffffffff81efcb42
RDX: 0000000000000000 RSI: ffff888093a97a08 RDI: ffff888093a977d8
RBP: ffff888093a97768 R08: ffff88809901c100 R09: ffff88809901c9c8
R10: ffff88809901c9a8 R11: ffff88809901c100 R12: 0000000000000001
R13: ffff8880995df4f0 R14: ffff888093a97740 R15: 0000000000000000
call_write_iter include/linux/fs.h:1872 [inline]
do_iter_readv_writev+0x5f8/0x8f0 fs/read_write.c:693
do_iter_write fs/read_write.c:970 [inline]
do_iter_write+0x184/0x610 fs/read_write.c:951
vfs_iter_write+0x77/0xb0 fs/read_write.c:983
iter_file_splice_write+0x65c/0xbd0 fs/splice.c:746
do_splice_from fs/splice.c:848 [inline]
direct_splice_actor+0x123/0x190 fs/splice.c:1020
splice_direct_to_actor+0x366/0x970 fs/splice.c:975
do_splice_direct+0x1da/0x2a0 fs/splice.c:1063
do_sendfile+0x597/0xd00 fs/read_write.c:1464
__do_sys_sendfile64 fs/read_write.c:1519 [inline]
__se_sys_sendfile64 fs/read_write.c:1511 [inline]
__x64_sys_sendfile64+0x15a/0x220 fs/read_write.c:1511
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x4417c9
Code: e8 7c e7 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 bb 07 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffce5c38198 EFLAGS: 00000246 ORIG_RAX: 0000000000000028
RAX: ffffffffffffffda RBX: 00007ffce5c38340 RCX: 00000000004417c9
RDX: 0000000020000000 RSI: 0000000000000003 RDI: 0000000000000003
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 00008080fffffffe R11: 0000000000000246 R12: 0000000000000000
R13: 00000000004024a0 R14: 0000000000000000 R15: 0000000000000000
rcu: rcu_preempt kthread starved for 10549 jiffies! g8969 f0x2
RCU_GP_WAIT_FQS(5) ->state=0x0 ->cpu=0
rcu: RCU grace-period kthread stack dump:
rcu_preempt R running task 29056 10 2 0x80004000
Call Trace:
context_switch kernel/sched/core.c:2818 [inline]
__schedule+0x7cb/0x1560 kernel/sched/core.c:3445
schedule+0xa8/0x260 kernel/sched/core.c:3509
schedule_timeout+0x486/0xc50 kernel/time/timer.c:1807
rcu_gp_fqs_loop kernel/rcu/tree.c:1589 [inline]
rcu_gp_kthread+0x9b2/0x18b0 kernel/rcu/tree.c:1746
kthread+0x354/0x420 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [PATCH v4 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Neil Horman @ 2019-06-26 17:14 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <CAF=yD-+_khMRCK0gE2q7nAi8fAtwvZ2FerHZKo1U1M-=991+Zg@mail.gmail.com>
On Wed, Jun 26, 2019 at 11:05:39AM -0400, Willem de Bruijn wrote:
> On Wed, Jun 26, 2019 at 6:54 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Tue, Jun 25, 2019 at 06:30:08PM -0400, Willem de Bruijn wrote:
> > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > index a29d66da7394..a7ca6a003ebe 100644
> > > > --- a/net/packet/af_packet.c
> > > > +++ b/net/packet/af_packet.c
> > > > @@ -2401,6 +2401,9 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
> > > >
> > > > ts = __packet_set_timestamp(po, ph, skb);
> > > > __packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
> > > > +
> > > > + if (!packet_read_pending(&po->tx_ring))
> > > > + complete(&po->skb_completion);
> > > > }
> > > >
> > > > sock_wfree(skb);
> > > > @@ -2585,7 +2588,7 @@ static int tpacket_parse_header(struct packet_sock *po, void *frame,
> > > >
> > > > static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > {
> > > > - struct sk_buff *skb;
> > > > + struct sk_buff *skb = NULL;
> > > > struct net_device *dev;
> > > > struct virtio_net_hdr *vnet_hdr = NULL;
> > > > struct sockcm_cookie sockc;
> > > > @@ -2600,6 +2603,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > int len_sum = 0;
> > > > int status = TP_STATUS_AVAILABLE;
> > > > int hlen, tlen, copylen = 0;
> > > > + long timeo = 0;
> > > >
> > > > mutex_lock(&po->pg_vec_lock);
> > > >
> > > > @@ -2646,12 +2650,21 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > if ((size_max > dev->mtu + reserve + VLAN_HLEN) && !po->has_vnet_hdr)
> > > > size_max = dev->mtu + reserve + VLAN_HLEN;
> > > >
> > > > + reinit_completion(&po->skb_completion);
> > > > +
> > > > do {
> > > > ph = packet_current_frame(po, &po->tx_ring,
> > > > TP_STATUS_SEND_REQUEST);
> > > > if (unlikely(ph == NULL)) {
> > > > - if (need_wait && need_resched())
> > > > - schedule();
> > > > + if (need_wait && skb) {
> > > > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > > > + timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
> > >
> > > This looks really nice.
> > >
> > > But isn't it still susceptible to the race where tpacket_destruct_skb
> > > is called in between po->xmit and this
> > > wait_for_completion_interruptible_timeout?
> > >
> > Thats not an issue, since the complete is only gated on packet_read_pending
> > reaching 0 in tpacket_destuct_skb. Previously it was gated on my
> > wait_on_complete flag being non-zero, so we had to set that prior to calling
> > po->xmit, or the complete call might never get made, resulting in a hang. Now,
> > we will always call complete, and the completion api allows for arbitrary
> > ordering of complete/wait_for_complete (since its internal done variable gets
> > incremented), making a call to wait_for_complete effectively a fall through if
> > complete gets called first.
>
> Perfect! I was not aware that it handles that internally. Hadn't read
> do_wait_for_common closely before.
>
> > There is an odd path here though. If an application calls sendmsg on a packet
> > socket here with MSG_DONTWAIT set, then need_wait will be zero, and we will
> > eventually exit this loop without ever having called wait_for_complete, but
> > tpacket_destruct_skb will still have called complete when all the frames
> > complete transmission. In and of itself, thats fine, but it leave the
> > completion structure in a state where its done variable will have been
> > incremented at least once (specifically it will be set to N, where N is the
> > number of frames transmitted during the call where MSG_DONTWAIT is set). If the
> > application then calls sendmsg on this socket with MSG_DONTWAIT clear, we will
> > call wait_for_complete, but immediately return from it (due to the previously
> > made calls to complete). I've corrected this however, but adding that call to
> > reinit_completion prior to the loop entry, so that we are always guaranteed to
> > have the completion variable set properly to wait for only the frames being sent
> > in this particular instance of the sendmsg call.
>
> Yep, understood.
>
> >
> > > The test for skb is shorthand for packet_read_pending != 0, right?
> > >
> > Sort of. gating on skb guarantees for us that we have sent at least one frame
> > in this call to tpacket_snd. If we didn't do that, then it would be possible
> > for an application to call sendmsg without setting any frames in the buffer to
> > TP_STATUS_SEND_REQUEST, which would cause us to wait for a completion without
> > having sent any frames, meaning we would block waiting for an event
> > (tpacket_destruct_skb), that will never happen. The check for skb ensures that
> > tpacket_snd_skb will get called, and that we will get a wakeup from a call to
> > wait_for_complete. It does suggest that packet_read_pending != 0, but thats not
> > guaranteed, because tpacket_destruct_skb may already have been called (see the
> > above explination regarding ordering of complete/wait_for_complete).
>
> But the inverse is true: if gating sleeping on packet_read_pending,
> the process only ever waits if a packet is still to be acknowledged.
> Then both the wait and wake clearly depend on the same state.
>
> Either way works, I think. So this is definitely fine.
>
Yeah, we could do that. Its basically a pick your poison situation, in the case
you stipulate, we could gate the wait_on_complete on read_pending being
non-zero, but if a frame frees quickly and decrements the pending count, we
still leave the loop with a completion struct that needs to be reset. Either
way we have to re-init the completion. And as for calling wait_on_complete when
we don't have to, your proposal does solve that prolbem but requires that we
call packet_read_pending an extra time for iteration on every loop.
Packet_read_pending accumulates the sum of all the per-cpu pointer pending
counts (which is a separate problem, I'm not sure why we're using per-cpu
counters there). Regardless, I looked at that and (anecdotally), decided that
periodically calling wait_for_complete which takes a spin lock would be more
performant than accessing a per-cpu variable on each available cpu every
iteration of the loop (based on the comments at the bottom of the loop).
> One possible refinement would be to keep po->wait_on_complete (but
> rename as po->wake_om_complete), set it before entering the loop and
> clear it before function return (both within the pg_vec_lock critical
> section). And test that in tpacket_destruct_skb to avoid calling
> complete if MSG_DONTWAIT. But I don't think it's worth the complexity.
>
I agree, we could use a socket variable to communicate to tpacket_destruct_skb
that we need to call complete, in conjunction with the pending count, but I
don't think the added complexity buys us anything.
> One rare edge case is a MSG_DONTWAIT send followed by a !MSG_DONTWAIT.
> It is then possible for a tpacket_destruct_skb to be run as a result
> from the first call, during the second call, after the call to
> reinit_completion. That would cause the next wait to return before
> *its* packets have been sent. But due to the packet_read_pending test
> in the while () condition it will loop again and return to wait. So that's fine.
>
yup, exactly.
> Thanks for bearing with me.
>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
>
^ permalink raw reply
* Re: [PATCH] team: Always enable vlan tx offload
From: David Miller @ 2019-06-26 17:14 UTC (permalink / raw)
To: yuehaibing
Cc: sdf, jianbol, jiri, mirq-linux, willemb, sdf, jiri, j.vosburgh,
vfalico, andy, linux-kernel, netdev
In-Reply-To: <20190626160339.35152-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com>
Date: Thu, 27 Jun 2019 00:03:39 +0800
> We should rather have vlan_tci filled all the way down
> to the transmitting netdevice and let it do the hw/sw
> vlan implementation.
>
> Suggested-by: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: pull-request: wireless-drivers-next 2019-06-26
From: David Miller @ 2019-06-26 17:12 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <87ef3gjq16.fsf@kamboji.qca.qualcomm.com>
From: Kalle Valo <kvalo@codeaurora.org>
Date: Wed, 26 Jun 2019 18:59:49 +0300
> here's a pull request to net-next for 5.3, more info below. Please let
> me know if there are any problems.
Pulled, thanks Kalo.
^ permalink raw reply
* Re: [PATCH net 0/2] net/smc: fixes 2019-06-26
From: David Miller @ 2019-06-26 17:10 UTC (permalink / raw)
To: ubraun
Cc: netdev, linux-s390, gor, heiko.carstens, raspl, kgraul, zhp,
yuehaibing
In-Reply-To: <20190626154750.46127-1-ubraun@linux.ibm.com>
From: Ursula Braun <ubraun@linux.ibm.com>
Date: Wed, 26 Jun 2019 17:47:48 +0200
> here are 2 small smc fixes for the net tree.
Looks good, series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next 17/18] ionic: Add RSS support
From: Shannon Nelson @ 2019-06-26 17:04 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev
In-Reply-To: <20190625172054.6a9d22dc@cakuba.netronome.com>
On 6/25/19 5:20 PM, Jakub Kicinski wrote:
> On Thu, 20 Jun 2019 13:24:23 -0700, Shannon Nelson wrote:
>> +static int ionic_lif_rss_init(struct lif *lif)
>> +{
>> + static const u8 toeplitz_symmetric_key[] = {
>> + 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A,
>> + 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A,
>> + 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A,
>> + 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A,
>> + 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A, 0x6D, 0x5A,
>> + };
> netdev_rss_key_fill()
Sure.
>
>> + unsigned int i, tbl_sz;
>> +
>> + lif->rss_types = IONIC_RSS_TYPE_IPV4 |
>> + IONIC_RSS_TYPE_IPV4_TCP |
>> + IONIC_RSS_TYPE_IPV4_UDP |
>> + IONIC_RSS_TYPE_IPV6 |
>> + IONIC_RSS_TYPE_IPV6_TCP |
>> + IONIC_RSS_TYPE_IPV6_UDP;
>> +
>> + /* Fill indirection table with 'default' values */
>> + tbl_sz = le16_to_cpu(lif->ionic->ident.lif.eth.rss_ind_tbl_sz);
>> + for (i = 0; i < tbl_sz; i++)
>> + lif->rss_ind_tbl[i] = i % lif->nxqs;
> ethtool_rxfh_indir_default()
Sure
>
>> + return ionic_lif_rss_config(lif, lif->rss_types,
>> + toeplitz_symmetric_key, NULL);
>> +}
Thanks for your time, I appreciate the review.
sln
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] net: sched: em_ipt: keep the user-specified nfproto and use it
From: nikolay @ 2019-06-26 17:02 UTC (permalink / raw)
To: Eyal Birger; +Cc: netdev, roopa, pablo, xiyou.wangcong, davem, jiri, jhs
In-Reply-To: <92B812B3-C5FB-452B-809A-1367349DB29A@cumulusnetworks.com>
On 26 June 2019 19:33:48 EEST, nikolay@cumulusnetworks.com wrote:
>On 26 June 2019 19:18:35 EEST, Eyal Birger <eyal.birger@gmail.com>
>wrote:
>>Hi Nik,
>>
>>On Wed, 26 Jun 2019 18:56:14 +0300
>>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>
>>> For NFPROTO_UNSPEC xt_matches there's no way to restrict the
>matching
>>> to a specific family, in order to do so we record the user-specified
>>> family and later enforce it while doing the match.
>>>
>>> v2: adjust changes to missing patch, was patch 04 in v1
>>>
>>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>> ---
>>> net/sched/em_ipt.c | 17 +++++++++++++++--
>>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>..snip..
>>> @@ -182,8 +195,8 @@ static int em_ipt_match(struct sk_buff *skb,
>>> struct tcf_ematch *em, const struct em_ipt_match *im = (const void
>>> *)em->data; struct xt_action_param acpar = {};
>>> struct net_device *indev = NULL;
>>> - u8 nfproto = im->match->family;
>>> struct nf_hook_state state;
>>> + u8 nfproto = im->nfproto;
>>
>>Maybe I'm missing something now - but it's not really clear to me now
>>why keeping im->nfproto would be useful:
>>
>>If NFPROTO_UNSPEC was provided by userspace then the actual nfproto
>>used
>>will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified
>>from userspace then it will equal im->match->family.
>>
>>Is there any case where the resulting nfproto would differ as a result
>>of this patch?
>>
>>Otherwise the patchset looks excellent to me.
>>
>>Thanks!
>>Eyal.
>
>Hi,
>It's needed to limit the match only to the user-specified family
>for unspec xt matches. The problem is otherwise im->match->family
>stays at nfproto_unspec regardless of the user choice.
>
>Thanks for reviewing the set.
>
>Cheers,
> Nik
Hm, while that is true, thinking more about it - mixing the user proto and the real proto
could be problematic since we no longer enforce them to be equal, but we check
the network header len based on the packet only and we can end up checking v4
len and parsing it as nfproto v6.
I'll spin v3 with unspec only and we can restrict it later if needed.
^ permalink raw reply
* Re: [PATCH net-next 14/18] ionic: Add Tx and Rx handling
From: Shannon Nelson @ 2019-06-26 16:49 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev
In-Reply-To: <20190625170803.2a23650b@cakuba.netronome.com>
On 6/25/19 5:08 PM, Jakub Kicinski wrote:
> On Thu, 20 Jun 2019 13:24:20 -0700, Shannon Nelson wrote:
>> Add both the Tx and Rx queue setup and handling. The related
>> stats display come later. Instead of using the generic napi
>> routines used by the slow-path command, the Tx and Rx paths
>> are simplified and inlined in one file in order to get better
>> compiler optimizations.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
>> index 5ebfaa320edf..6dfcada9e822 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
>> @@ -351,6 +351,54 @@ int ionic_debugfs_add_qcq(struct lif *lif, struct qcq *qcq)
>> desc_blob);
>> }
>>
>> + if (qcq->flags & QCQ_F_TX_STATS) {
>> + stats_dentry = debugfs_create_dir("tx_stats", q_dentry);
>> + if (IS_ERR_OR_NULL(stats_dentry))
>> + return PTR_ERR(stats_dentry);
>> +
>> + debugfs_create_u64("dma_map_err", 0400, stats_dentry,
>> + &qcq->stats->tx.dma_map_err);
>> + debugfs_create_u64("pkts", 0400, stats_dentry,
>> + &qcq->stats->tx.pkts);
>> + debugfs_create_u64("bytes", 0400, stats_dentry,
>> + &qcq->stats->tx.bytes);
>> + debugfs_create_u64("clean", 0400, stats_dentry,
>> + &qcq->stats->tx.clean);
>> + debugfs_create_u64("linearize", 0400, stats_dentry,
>> + &qcq->stats->tx.linearize);
>> + debugfs_create_u64("no_csum", 0400, stats_dentry,
>> + &qcq->stats->tx.no_csum);
>> + debugfs_create_u64("csum", 0400, stats_dentry,
>> + &qcq->stats->tx.csum);
>> + debugfs_create_u64("crc32_csum", 0400, stats_dentry,
>> + &qcq->stats->tx.crc32_csum);
>> + debugfs_create_u64("tso", 0400, stats_dentry,
>> + &qcq->stats->tx.tso);
>> + debugfs_create_u64("frags", 0400, stats_dentry,
>> + &qcq->stats->tx.frags);
> I wonder why debugfs over ethtool -S?
I believe this was from early engineering, before ethtool -S had been
filled out. I'll clean that up.
>
>> +static int ionic_tx(struct queue *q, struct sk_buff *skb)
>> +{
>> + struct tx_stats *stats = q_to_tx_stats(q);
>> + int err;
>> +
>> + if (skb->ip_summed == CHECKSUM_PARTIAL)
>> + err = ionic_tx_calc_csum(q, skb);
>> + else
>> + err = ionic_tx_calc_no_csum(q, skb);
>> + if (err)
>> + return err;
>> +
>> + err = ionic_tx_skb_frags(q, skb);
>> + if (err)
>> + return err;
>> +
>> + skb_tx_timestamp(skb);
>> + stats->pkts++;
>> + stats->bytes += skb->len;
> nit: I think counting stats on completion may be a better idea,
> otherwise when you can a full ring on stop your HW counters are
> guaranteed to be different than SW counters. Am I wrong?
You are not wrong, that is how many drivers handle it. I like seeing
how much the driver was given (ethtool -S) versus how much the HW
actually pushed out (netstat -i or ip -s link show). These numbers
shouldn't be very often be very different, but it is interesting when
they are.
>
>> + ionic_txq_post(q, !netdev_xmit_more(), ionic_tx_clean, skb);
>> +
>> + return 0;
>> +}
>> +
>> +static int ionic_tx_descs_needed(struct queue *q, struct sk_buff *skb)
>> +{
>> + struct tx_stats *stats = q_to_tx_stats(q);
>> + int err;
>> +
>> + /* If TSO, need roundup(skb->len/mss) descs */
>> + if (skb_is_gso(skb))
>> + return (skb->len / skb_shinfo(skb)->gso_size) + 1;
> This doesn't look correct, are you sure you don't want
> skb_shinfo(skb)->gso_segs ?
That would probably work as well.
>
>> +
>> + /* If non-TSO, just need 1 desc and nr_frags sg elems */
>> + if (skb_shinfo(skb)->nr_frags <= IONIC_TX_MAX_SG_ELEMS)
>> + return 1;
>> +
>> + /* Too many frags, so linearize */
>> + err = skb_linearize(skb);
>> + if (err)
>> + return err;
>> +
>> + stats->linearize++;
>> +
>> + /* Need 1 desc and zero sg elems */
>> + return 1;
>> +}
>> +
>> +netdev_tx_t ionic_start_xmit(struct sk_buff *skb, struct net_device *netdev)
>> +{
>> + u16 queue_index = skb_get_queue_mapping(skb);
>> + struct lif *lif = netdev_priv(netdev);
>> + struct queue *q;
>> + int ndescs;
>> + int err;
>> +
>> + if (unlikely(!test_bit(LIF_UP, lif->state))) {
>> + dev_kfree_skb(skb);
>> + return NETDEV_TX_OK;
>> + }
> Surely you stop TX before taking the queues down?
Yes, in ionic_lif_stop()
>
>> + if (likely(lif_to_txqcq(lif, queue_index)))
>> + q = lif_to_txq(lif, queue_index);
>> + else
>> + q = lif_to_txq(lif, 0);
>> +
>> + ndescs = ionic_tx_descs_needed(q, skb);
>> + if (ndescs < 0)
>> + goto err_out_drop;
>> +
>> + if (!ionic_q_has_space(q, ndescs)) {
>> + netif_stop_subqueue(netdev, queue_index);
>> + q->stop++;
>> +
>> + /* Might race with ionic_tx_clean, check again */
>> + smp_rmb();
>> + if (ionic_q_has_space(q, ndescs)) {
>> + netif_wake_subqueue(netdev, queue_index);
>> + q->wake++;
>> + } else {
>> + return NETDEV_TX_BUSY;
>> + }
>> + }
>> +
>> + if (skb_is_gso(skb))
>> + err = ionic_tx_tso(q, skb);
>> + else
>> + err = ionic_tx(q, skb);
>> +
>> + if (err)
>> + goto err_out_drop;
>> +
>> + return NETDEV_TX_OK;
>> +
>> +err_out_drop:
>> + q->drop++;
>> + dev_kfree_skb(skb);
>> + return NETDEV_TX_OK;
>> +}
^ 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