* Re: [PATCH net-next 00/15] mlxsw: Offload IPv6 multicast routes
From: David Miller @ 2018-03-26 17:15 UTC (permalink / raw)
To: idosch; +Cc: netdev, yuvalm, jiri, nikolay
In-Reply-To: <20180326120145.11752-1-idosch@mellanox.com>
From: Ido Schimmel <idosch@mellanox.com>
Date: Mon, 26 Mar 2018 15:01:30 +0300
> The series is intended to allow offloading IPv6 multicast routes
> and is split into two parts:
>
> - First half of the patches continue extending ip6mr [& refactor ipmr]
> with missing bits necessary for the offloading - fib-notifications,
> mfc refcounting and default rule identification.
>
> - Second half of the patches extend functionality inside mlxsw,
> beginning with extending lower-parts to support IPv6 mroutes
> to host and later extending the router/mr internal APIs within
> the driver to accommodate support in ipv6 configurations.
> Lastly it adds support in the RTNL_FAMILY_IP6MR notifications,
> allowing driver to react and offload related routes.
I like the consolidations and the rest of the series looks fine to me
as well, so series applied.
Any adjustments people want can be done as follow-on patches.
Thank you.
^ permalink raw reply
* [PATCH v2 net] r8169: fix setting driver_data after register_netdev
From: Heiner Kallweit @ 2018-03-26 17:19 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
pci_set_drvdata() is called only after registering the net_device,
therefore we could run into a NPE if one of the functions using
driver_data is called before it's set.
Fix this by calling pci_set_drvdata() before registering the
net_device.
This fix is a candidate for stable. As far as I can see the
bug has been there in kernel version 3.2 already, therefore
I can't provide a reference which commit is fixed by it.
The fix may need small adjustments per kernel version because
due to other changes the label which is jumped to if
register_netdev() fails has changed over time.
Reported-by: David Miller <davem@davemloft.net>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased based on net instead of net-next
---
drivers/net/ethernet/realtek/r8169.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0bf7d1759..b4779acb6 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -8660,12 +8660,12 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
if (!tp->counters)
return -ENOMEM;
+ pci_set_drvdata(pdev, dev);
+
rc = register_netdev(dev);
if (rc < 0)
return rc;
- pci_set_drvdata(pdev, dev);
-
netif_info(tp, probe, dev, "%s at 0x%p, %pM, XID %08x IRQ %d\n",
rtl_chip_infos[chipset].name, ioaddr, dev->dev_addr,
(u32)(RTL_R32(TxConfig) & 0x9cf0f8ff), pdev->irq);
--
2.16.2
^ permalink raw reply related
* [bpf-next PATCH 0/4] bpf, sockmap BPF_F_INGRESS support
From: John Fastabend @ 2018-03-26 17:22 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
This series adds the BPF_F_INGRESS flag support to the reidrect APIs.
Bringing the sockmap API in-line with the cls_bpf redirect APIs.
We add it to both variants of sockmap programs, the first patch adds
support for tx ulp hooks and the third patch adds supoprt for the recv
skb hooks. Patches two and four add tests for the corresponding
ingress redirect hooks.
Follow on patches can address busy polling support, but next series
from me will move the sockmap sample program into selftests.
Thanks,
John
---
John Fastabend (4):
bpf: sockmap redirect ingress support
bpf: sockmap, add BPF_F_INGRESS tests
bpf: sockmap, BPF_F_INGRESS flag for BPF_SK_SKB_STREAM_VERDICT:
bpf: sockmap, more BPF_SK_SKB_STREAM_VERDICT tests
include/linux/filter.h | 2
include/net/sock.h | 1
kernel/bpf/sockmap.c | 290 ++++++++++++++++++++++++++++++++++++---
net/core/filter.c | 4 -
net/ipv4/tcp.c | 10 +
samples/sockmap/sockmap_kern.c | 62 +++++++-
samples/sockmap/sockmap_test.sh | 40 +++++
samples/sockmap/sockmap_user.c | 58 ++++++++
8 files changed, 430 insertions(+), 37 deletions(-)
^ permalink raw reply
* [bpf-next PATCH 1/4] bpf: sockmap redirect ingress support
From: John Fastabend @ 2018-03-26 17:22 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
In-Reply-To: <20180326172022.20504.1790.stgit@john-Precision-Tower-5810>
Add support for the BPF_F_INGRESS flag in sk_msg redirect helper.
To do this add a scatterlist ring for receiving socks to check
before calling into regular recvmsg call path. Additionally, because
the poll wakeup logic only checked the skb recv queue we need to
add a hook in TCP stack (similar to write side) so that we have
a way to wake up polling socks when a scatterlist is redirected
to that sock.
After this all that is needed is for the redirect helper to
push the scatterlist into the psock receive queue.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/linux/filter.h | 1
include/net/sock.h | 1
kernel/bpf/sockmap.c | 198 +++++++++++++++++++++++++++++++++++++++++++++++-
net/core/filter.c | 2
net/ipv4/tcp.c | 10 ++
5 files changed, 207 insertions(+), 5 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 109d05c..d0e207f 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -521,6 +521,7 @@ struct sk_msg_buff {
__u32 key;
__u32 flags;
struct bpf_map *map;
+ struct list_head list;
};
/* Compute the linear packet data range [data, data_end) which
diff --git a/include/net/sock.h b/include/net/sock.h
index 7093111..b8ff435 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1085,6 +1085,7 @@ struct proto {
#endif
bool (*stream_memory_free)(const struct sock *sk);
+ bool (*stream_memory_read)(const struct sock *sk);
/* Memory pressure */
void (*enter_memory_pressure)(struct sock *sk);
void (*leave_memory_pressure)(struct sock *sk);
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 69c5bcc..7f32b03 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -41,6 +41,8 @@
#include <linux/mm.h>
#include <net/strparser.h>
#include <net/tcp.h>
+#include <linux/ptr_ring.h>
+#include <net/inet_common.h>
#define SOCK_CREATE_FLAG_MASK \
(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
@@ -82,6 +84,7 @@ struct smap_psock {
int sg_size;
int eval;
struct sk_msg_buff *cork;
+ struct list_head ingress;
struct strparser strp;
struct bpf_prog *bpf_tx_msg;
@@ -103,6 +106,8 @@ struct smap_psock {
};
static void smap_release_sock(struct smap_psock *psock, struct sock *sock);
+static int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
+ int nonblock, int flags, int *addr_len);
static int bpf_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size);
static int bpf_tcp_sendpage(struct sock *sk, struct page *page,
int offset, size_t size, int flags);
@@ -112,6 +117,21 @@ static inline struct smap_psock *smap_psock_sk(const struct sock *sk)
return rcu_dereference_sk_user_data(sk);
}
+bool bpf_tcp_stream_read(const struct sock *sk)
+{
+ struct smap_psock *psock;
+ bool empty = true;
+
+ rcu_read_lock();
+ psock = smap_psock_sk(sk);
+ if (unlikely(!psock))
+ goto out;
+ empty = list_empty(&psock->ingress);
+out:
+ rcu_read_unlock();
+ return !empty;
+}
+
static struct proto tcp_bpf_proto;
static int bpf_tcp_init(struct sock *sk)
{
@@ -135,6 +155,8 @@ static int bpf_tcp_init(struct sock *sk)
if (psock->bpf_tx_msg) {
tcp_bpf_proto.sendmsg = bpf_tcp_sendmsg;
tcp_bpf_proto.sendpage = bpf_tcp_sendpage;
+ tcp_bpf_proto.recvmsg = bpf_tcp_recvmsg;
+ tcp_bpf_proto.stream_memory_read = bpf_tcp_stream_read;
}
sk->sk_prot = &tcp_bpf_proto;
@@ -170,6 +192,7 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
{
void (*close_fun)(struct sock *sk, long timeout);
struct smap_psock_map_entry *e, *tmp;
+ struct sk_msg_buff *md, *mtmp;
struct smap_psock *psock;
struct sock *osk;
@@ -188,6 +211,12 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
close_fun = psock->save_close;
write_lock_bh(&sk->sk_callback_lock);
+ list_for_each_entry_safe(md, mtmp, &psock->ingress, list) {
+ list_del(&md->list);
+ free_start_sg(psock->sock, md);
+ kfree(md);
+ }
+
list_for_each_entry_safe(e, tmp, &psock->maps, list) {
osk = cmpxchg(e->entry, sk, NULL);
if (osk == sk) {
@@ -468,6 +497,72 @@ static unsigned int smap_do_tx_msg(struct sock *sk,
return _rc;
}
+static int bpf_tcp_ingress(struct sock *sk, int apply_bytes,
+ struct smap_psock *psock,
+ struct sk_msg_buff *md, int flags)
+{
+ bool apply = apply_bytes;
+ size_t size, copied = 0;
+ struct sk_msg_buff *r;
+ int err = 0, i;
+
+ r = kzalloc(sizeof(struct sk_msg_buff), __GFP_NOWARN | GFP_KERNEL);
+ if (unlikely(!r))
+ return -ENOMEM;
+
+ lock_sock(sk);
+ r->sg_start = md->sg_start;
+ i = md->sg_start;
+
+ do {
+ r->sg_data[i] = md->sg_data[i];
+
+ size = (apply && apply_bytes < md->sg_data[i].length) ?
+ apply_bytes : md->sg_data[i].length;
+
+ if (!sk_wmem_schedule(sk, size)) {
+ if (!copied)
+ err = -ENOMEM;
+ break;
+ }
+
+ sk_mem_charge(sk, size);
+ r->sg_data[i].length = size;
+ md->sg_data[i].length -= size;
+ md->sg_data[i].offset += size;
+ copied += size;
+
+ if (md->sg_data[i].length) {
+ get_page(sg_page(&r->sg_data[i]));
+ r->sg_end = (i + 1) == MAX_SKB_FRAGS ? 0 : i + 1;
+ } else {
+ i++;
+ if (i == MAX_SKB_FRAGS)
+ i = 0;
+ r->sg_end = i;
+ }
+
+ if (apply) {
+ apply_bytes -= size;
+ if (!apply_bytes)
+ break;
+ }
+ } while (i != md->sg_end);
+
+ md->sg_start = i;
+
+ if (!err) {
+ list_add_tail(&r->list, &psock->ingress);
+ sk->sk_data_ready(sk);
+ } else {
+ free_start_sg(sk, r);
+ kfree(r);
+ }
+
+ release_sock(sk);
+ return err;
+}
+
static int bpf_tcp_sendmsg_do_redirect(struct sock *sk, int send,
struct sk_msg_buff *md,
int flags)
@@ -475,6 +570,7 @@ static int bpf_tcp_sendmsg_do_redirect(struct sock *sk, int send,
struct smap_psock *psock;
struct scatterlist *sg;
int i, err, free = 0;
+ bool ingress = !!(md->flags & BPF_F_INGRESS);
sg = md->sg_data;
@@ -487,9 +583,14 @@ static int bpf_tcp_sendmsg_do_redirect(struct sock *sk, int send,
goto out_rcu;
rcu_read_unlock();
- lock_sock(sk);
- err = bpf_tcp_push(sk, send, md, flags, false);
- release_sock(sk);
+
+ if (ingress) {
+ err = bpf_tcp_ingress(sk, send, psock, md, flags);
+ } else {
+ lock_sock(sk);
+ err = bpf_tcp_push(sk, send, md, flags, false);
+ release_sock(sk);
+ }
smap_release_sock(psock, sk);
if (unlikely(err))
goto out;
@@ -623,6 +724,89 @@ static int bpf_exec_tx_verdict(struct smap_psock *psock,
return err;
}
+int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
+ int nonblock, int flags, int *addr_len)
+{
+ struct iov_iter *iter = &msg->msg_iter;
+ struct smap_psock *psock;
+ int copied = 0;
+
+ if (unlikely(flags & MSG_ERRQUEUE))
+ return inet_recv_error(sk, msg, len, addr_len);
+
+ rcu_read_lock();
+ psock = smap_psock_sk(sk);
+ if (unlikely(!psock))
+ goto out;
+
+ if (unlikely(!refcount_inc_not_zero(&psock->refcnt)))
+ goto out;
+
+ if (!skb_queue_empty(&sk->sk_receive_queue))
+ goto out;
+ rcu_read_unlock();
+
+ lock_sock(sk);
+ while (copied != len) {
+ struct scatterlist *sg;
+ struct sk_msg_buff *md;
+ int i;
+
+ md = list_first_entry_or_null(&psock->ingress,
+ struct sk_msg_buff, list);
+ if (unlikely(!md))
+ break;
+ i = md->sg_start;
+ do {
+ struct page *page;
+ int n, copy;
+
+ sg = &md->sg_data[i];
+ copy = sg->length;
+ page = sg_page(sg);
+
+ if (copied + copy > len)
+ copy = len - copied;
+
+ n = copy_page_to_iter(page, sg->offset, copy, iter);
+ if (n != copy) {
+ md->sg_start = i;
+ release_sock(sk);
+ smap_release_sock(psock, sk);
+ return -EFAULT;
+ }
+
+ copied += copy;
+ sg->offset += copy;
+ sg->length -= copy;
+ sk_mem_uncharge(sk, copy);
+
+ if (!sg->length) {
+ i++;
+ if (i == MAX_SKB_FRAGS)
+ i = 0;
+ put_page(page);
+ }
+ if (copied == len)
+ break;
+ } while (i != md->sg_end);
+ md->sg_start = i;
+
+ if (!sg->length && md->sg_start == md->sg_end) {
+ list_del(&md->list);
+ kfree(md);
+ }
+ }
+
+ release_sock(sk);
+ smap_release_sock(psock, sk);
+ return copied;
+out:
+ rcu_read_unlock();
+ return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
+}
+
+
static int bpf_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
{
int flags = msg->msg_flags | MSG_NO_SHARED_FRAGS;
@@ -1107,6 +1291,7 @@ static void sock_map_remove_complete(struct bpf_stab *stab)
static void smap_gc_work(struct work_struct *w)
{
struct smap_psock_map_entry *e, *tmp;
+ struct sk_msg_buff *md, *mtmp;
struct smap_psock *psock;
psock = container_of(w, struct smap_psock, gc_work);
@@ -1131,6 +1316,12 @@ static void smap_gc_work(struct work_struct *w)
kfree(psock->cork);
}
+ list_for_each_entry_safe(md, mtmp, &psock->ingress, list) {
+ list_del(&md->list);
+ free_start_sg(psock->sock, md);
+ kfree(md);
+ }
+
list_for_each_entry_safe(e, tmp, &psock->maps, list) {
list_del(&e->list);
kfree(e);
@@ -1160,6 +1351,7 @@ static struct smap_psock *smap_init_psock(struct sock *sock,
INIT_WORK(&psock->tx_work, smap_tx_work);
INIT_WORK(&psock->gc_work, smap_gc_work);
INIT_LIST_HEAD(&psock->maps);
+ INIT_LIST_HEAD(&psock->ingress);
refcount_set(&psock->refcnt, 1);
rcu_assign_sk_user_data(sock, psock);
diff --git a/net/core/filter.c b/net/core/filter.c
index 00c711c..11b1f16 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1894,7 +1894,7 @@ struct sock *do_sk_redirect_map(struct sk_buff *skb)
struct bpf_map *, map, u32, key, u64, flags)
{
/* If user passes invalid input drop the packet. */
- if (unlikely(flags))
+ if (unlikely(flags & ~(BPF_F_INGRESS)))
return SK_DROP;
msg->key = key;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 0c31be3..bccc4c2 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -485,6 +485,14 @@ static void tcp_tx_timestamp(struct sock *sk, u16 tsflags)
}
}
+static inline bool tcp_stream_is_readable(const struct tcp_sock *tp,
+ int target, struct sock *sk)
+{
+ return (tp->rcv_nxt - tp->copied_seq >= target) ||
+ (sk->sk_prot->stream_memory_read ?
+ sk->sk_prot->stream_memory_read(sk) : false);
+}
+
/*
* Wait for a TCP event.
*
@@ -554,7 +562,7 @@ __poll_t tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
tp->urg_data)
target++;
- if (tp->rcv_nxt - tp->copied_seq >= target)
+ if (tcp_stream_is_readable(tp, target, sk))
mask |= EPOLLIN | EPOLLRDNORM;
if (!(sk->sk_shutdown & SEND_SHUTDOWN)) {
^ permalink raw reply related
* [bpf-next PATCH 2/4] bpf: sockmap, add BPF_F_INGRESS tests
From: John Fastabend @ 2018-03-26 17:22 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
In-Reply-To: <20180326172022.20504.1790.stgit@john-Precision-Tower-5810>
Add a set of tests to verify ingress flag in redirect helpers
works correctly with various msg sizes.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
samples/sockmap/sockmap_kern.c | 41 +++++++++++++++++++++++++++++----------
samples/sockmap/sockmap_test.sh | 22 ++++++++++++++++++++-
samples/sockmap/sockmap_user.c | 35 +++++++++++++++++++++++++++++++++
3 files changed, 87 insertions(+), 11 deletions(-)
diff --git a/samples/sockmap/sockmap_kern.c b/samples/sockmap/sockmap_kern.c
index 9ad5ba7..ca28722 100644
--- a/samples/sockmap/sockmap_kern.c
+++ b/samples/sockmap/sockmap_kern.c
@@ -54,7 +54,7 @@ struct bpf_map_def SEC("maps") sock_map_redir = {
.type = BPF_MAP_TYPE_SOCKMAP,
.key_size = sizeof(int),
.value_size = sizeof(int),
- .max_entries = 1,
+ .max_entries = 20,
};
struct bpf_map_def SEC("maps") sock_apply_bytes = {
@@ -78,6 +78,13 @@ struct bpf_map_def SEC("maps") sock_pull_bytes = {
.max_entries = 2
};
+struct bpf_map_def SEC("maps") sock_redir_flags = {
+ .type = BPF_MAP_TYPE_ARRAY,
+ .key_size = sizeof(int),
+ .value_size = sizeof(int),
+ .max_entries = 1
+};
+
SEC("sk_skb1")
int bpf_prog1(struct __sk_buff *skb)
@@ -197,8 +204,9 @@ int bpf_prog5(struct sk_msg_md *msg)
SEC("sk_msg3")
int bpf_prog6(struct sk_msg_md *msg)
{
- int *bytes, zero = 0, one = 1;
- int *start, *end;
+ int *bytes, zero = 0, one = 1, key = 0;
+ int *start, *end, *f;
+ __u64 flags = 0;
bytes = bpf_map_lookup_elem(&sock_apply_bytes, &zero);
if (bytes)
@@ -210,15 +218,22 @@ int bpf_prog6(struct sk_msg_md *msg)
end = bpf_map_lookup_elem(&sock_pull_bytes, &one);
if (start && end)
bpf_msg_pull_data(msg, *start, *end, 0);
- return bpf_msg_redirect_map(msg, &sock_map_redir, zero, 0);
+ f = bpf_map_lookup_elem(&sock_redir_flags, &zero);
+ if (f && *f) {
+ key = 2;
+ flags = *f;
+ }
+ return bpf_msg_redirect_map(msg, &sock_map_redir, key, flags);
}
SEC("sk_msg4")
int bpf_prog7(struct sk_msg_md *msg)
{
- int err1 = 0, err2 = 0, zero = 0, one = 1;
- int *bytes, *start, *end, len1, len2;
+ int err1 = 0, err2 = 0, zero = 0, one = 1, key = 0;
+ int *f, *bytes, *start, *end, len1, len2;
+ __u64 flags = 0;
+ int err;
bytes = bpf_map_lookup_elem(&sock_apply_bytes, &zero);
if (bytes)
err1 = bpf_msg_apply_bytes(msg, *bytes);
@@ -229,7 +244,6 @@ int bpf_prog7(struct sk_msg_md *msg)
start = bpf_map_lookup_elem(&sock_pull_bytes, &zero);
end = bpf_map_lookup_elem(&sock_pull_bytes, &one);
if (start && end) {
- int err;
bpf_printk("sk_msg2: pull(%i:%i)\n",
start ? *start : 0, end ? *end : 0);
@@ -241,9 +255,16 @@ int bpf_prog7(struct sk_msg_md *msg)
bpf_printk("sk_msg2: length update %i->%i\n",
len1, len2);
}
- bpf_printk("sk_msg3: redirect(%iB) err1=%i err2=%i\n",
- len1, err1, err2);
- return bpf_msg_redirect_map(msg, &sock_map_redir, zero, 0);
+ f = bpf_map_lookup_elem(&sock_redir_flags, &zero);
+ if (f && *f) {
+ key = 2;
+ flags = *f;
+ }
+ bpf_printk("sk_msg3: redirect(%iB) flags=%i err=%i\n",
+ len1, flags, err1 ? err1 : err2);
+ err = bpf_msg_redirect_map(msg, &sock_map_redir, key, flags);
+ bpf_printk("sk_msg3: err %i\n", err);
+ return err;
}
SEC("sk_msg5")
diff --git a/samples/sockmap/sockmap_test.sh b/samples/sockmap/sockmap_test.sh
index 6d8cc40..13b205f 100755
--- a/samples/sockmap/sockmap_test.sh
+++ b/samples/sockmap/sockmap_test.sh
@@ -1,5 +1,5 @@
#Test a bunch of positive cases to verify basic functionality
-for prog in "--txmsg" "--txmsg_redir" "--txmsg_drop"; do
+for prog in "--txmsg_redir --txmsg_ingress" "--txmsg" "--txmsg_redir" "--txmsg_redir --txmsg_ingress" "--txmsg_drop"; do
for t in "sendmsg" "sendpage"; do
for r in 1 10 100; do
for i in 1 10 100; do
@@ -100,6 +100,16 @@ for t in "sendmsg" "sendpage"; do
sleep 2
done
+prog="--txmsg_redir --txmsg_apply 1 --txmsg_ingress"
+
+for t in "sendmsg" "sendpage"; do
+ TEST="./sockmap --cgroup /mnt/cgroup2/ -t $t -r $r -i $i -l $l $prog"
+ echo $TEST
+ $TEST
+ sleep 2
+done
+
+
# Test apply and redirect with larger value than send
r=1
i=8
@@ -113,6 +123,16 @@ for t in "sendmsg" "sendpage"; do
sleep 2
done
+prog="--txmsg_redir --txmsg_apply 2048 --txmsg_ingress"
+
+for t in "sendmsg" "sendpage"; do
+ TEST="./sockmap --cgroup /mnt/cgroup2/ -t $t -r $r -i $i -l $l $prog"
+ echo $TEST
+ $TEST
+ sleep 2
+done
+
+
# Test apply and redirect with apply that never reaches limit
r=1024
i=1
diff --git a/samples/sockmap/sockmap_user.c b/samples/sockmap/sockmap_user.c
index 07aa237..f7503f4 100644
--- a/samples/sockmap/sockmap_user.c
+++ b/samples/sockmap/sockmap_user.c
@@ -64,6 +64,7 @@
int txmsg_cork;
int txmsg_start;
int txmsg_end;
+int txmsg_ingress;
static const struct option long_options[] = {
{"help", no_argument, NULL, 'h' },
@@ -83,6 +84,7 @@
{"txmsg_cork", required_argument, NULL, 'k'},
{"txmsg_start", required_argument, NULL, 's'},
{"txmsg_end", required_argument, NULL, 'e'},
+ {"txmsg_ingress", no_argument, &txmsg_ingress, 1 },
{0, 0, NULL, 0 }
};
@@ -793,6 +795,39 @@ int main(int argc, char **argv)
return err;
}
}
+
+ if (txmsg_ingress) {
+ int in = BPF_F_INGRESS;
+
+ i = 0;
+ err = bpf_map_update_elem(map_fd[6], &i, &in, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (txmsg_ingress): %d (%s)\n",
+ err, strerror(errno));
+ }
+ i = 1;
+ err = bpf_map_update_elem(map_fd[1], &i, &p1, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (p1 txmsg): %d (%s)\n",
+ err, strerror(errno));
+ }
+ err = bpf_map_update_elem(map_fd[2], &i, &p1, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (p1 redir): %d (%s)\n",
+ err, strerror(errno));
+ }
+
+ i = 2;
+ err = bpf_map_update_elem(map_fd[2], &i, &p2, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (p2 txmsg): %d (%s)\n",
+ err, strerror(errno));
+ }
+ }
}
if (txmsg_drop)
^ permalink raw reply related
* [bpf-next PATCH 3/4] bpf: sockmap, BPF_F_INGRESS flag for BPF_SK_SKB_STREAM_VERDICT:
From: John Fastabend @ 2018-03-26 17:22 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
In-Reply-To: <20180326172022.20504.1790.stgit@john-Precision-Tower-5810>
Add support for the BPF_F_INGRESS flag in skb redirect helper. To
do this convert skb into a scatterlist and push into ingress queue.
This is the same logic that is used in the sk_msg redirect helper
so it should feel familiar.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/linux/filter.h | 1 +
kernel/bpf/sockmap.c | 94 +++++++++++++++++++++++++++++++++++++++---------
net/core/filter.c | 2 +
3 files changed, 78 insertions(+), 19 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index d0e207f..7de778a 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -521,6 +521,7 @@ struct sk_msg_buff {
__u32 key;
__u32 flags;
struct bpf_map *map;
+ struct sk_buff *skb;
struct list_head list;
};
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 7f32b03..92b4e4b 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -785,7 +785,8 @@ int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
i++;
if (i == MAX_SKB_FRAGS)
i = 0;
- put_page(page);
+ if (!md->skb)
+ put_page(page);
}
if (copied == len)
break;
@@ -794,6 +795,8 @@ int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
if (!sg->length && md->sg_start == md->sg_end) {
list_del(&md->list);
+ if (md->skb)
+ consume_skb(md->skb);
kfree(md);
}
}
@@ -1045,27 +1048,72 @@ static int smap_verdict_func(struct smap_psock *psock, struct sk_buff *skb)
__SK_DROP;
}
+static int smap_do_ingress(struct smap_psock *psock, struct sk_buff *skb)
+{
+ struct sock *sk = psock->sock;
+ int copied = 0, num_sg;
+ struct sk_msg_buff *r;
+
+ r = kzalloc(sizeof(struct sk_msg_buff), __GFP_NOWARN | GFP_ATOMIC);
+ if (unlikely(!r))
+ return -EAGAIN;
+
+ if (!sk_rmem_schedule(sk, skb, skb->len)) {
+ kfree(r);
+ return -EAGAIN;
+ }
+ sk_mem_charge(sk, skb->len);
+
+ sg_init_table(r->sg_data, MAX_SKB_FRAGS);
+ num_sg = skb_to_sgvec(skb, r->sg_data, 0, skb->len);
+ if (unlikely(num_sg < 0)) {
+ kfree(r);
+ return num_sg;
+ }
+ copied = skb->len;
+ r->sg_start = 0;
+ r->sg_end = num_sg == MAX_SKB_FRAGS ? 0 : num_sg;
+ r->skb = skb;
+ list_add_tail(&r->list, &psock->ingress);
+ sk->sk_data_ready(sk);
+ return copied;
+}
+
static void smap_do_verdict(struct smap_psock *psock, struct sk_buff *skb)
{
+ struct smap_psock *peer;
struct sock *sk;
+ __u32 in;
int rc;
rc = smap_verdict_func(psock, skb);
switch (rc) {
case __SK_REDIRECT:
sk = do_sk_redirect_map(skb);
- if (likely(sk)) {
- struct smap_psock *peer = smap_psock_sk(sk);
-
- if (likely(peer &&
- test_bit(SMAP_TX_RUNNING, &peer->state) &&
- !sock_flag(sk, SOCK_DEAD) &&
- sock_writeable(sk))) {
- skb_set_owner_w(skb, sk);
- skb_queue_tail(&peer->rxqueue, skb);
- schedule_work(&peer->tx_work);
- break;
- }
+ if (!sk) {
+ kfree_skb(skb);
+ break;
+ }
+
+ peer = smap_psock_sk(sk);
+ in = (TCP_SKB_CB(skb)->bpf.flags) & BPF_F_INGRESS;
+
+ if (unlikely(!peer || sock_flag(sk, SOCK_DEAD) ||
+ !test_bit(SMAP_TX_RUNNING, &peer->state))) {
+ kfree_skb(skb);
+ break;
+ }
+
+ if (!in && sock_writeable(sk)) {
+ skb_set_owner_w(skb, sk);
+ skb_queue_tail(&peer->rxqueue, skb);
+ schedule_work(&peer->tx_work);
+ break;
+ } else if (in &&
+ atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf) {
+ skb_queue_tail(&peer->rxqueue, skb);
+ schedule_work(&peer->tx_work);
+ break;
}
/* Fall through and free skb otherwise */
case __SK_DROP:
@@ -1127,15 +1175,23 @@ static void smap_tx_work(struct work_struct *w)
}
while ((skb = skb_dequeue(&psock->rxqueue))) {
+ __u32 flags;
+
rem = skb->len;
off = 0;
start:
+ flags = (TCP_SKB_CB(skb)->bpf.flags) & BPF_F_INGRESS;
do {
- if (likely(psock->sock->sk_socket))
- n = skb_send_sock_locked(psock->sock,
- skb, off, rem);
- else
+ if (likely(psock->sock->sk_socket)) {
+ if (flags)
+ n = smap_do_ingress(psock, skb);
+ else
+ n = skb_send_sock_locked(psock->sock,
+ skb, off, rem);
+ } else {
n = -EINVAL;
+ }
+
if (n <= 0) {
if (n == -EAGAIN) {
/* Retry when space is available */
@@ -1153,7 +1209,9 @@ static void smap_tx_work(struct work_struct *w)
rem -= n;
off += n;
} while (rem);
- kfree_skb(skb);
+
+ if (!flags)
+ kfree_skb(skb);
}
out:
release_sock(psock->sock);
diff --git a/net/core/filter.c b/net/core/filter.c
index 11b1f16..b46916d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1855,7 +1855,7 @@ int skb_do_redirect(struct sk_buff *skb)
struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
/* If user passes invalid input drop the packet. */
- if (unlikely(flags))
+ if (unlikely(flags & ~(BPF_F_INGRESS)))
return SK_DROP;
tcb->bpf.key = key;
^ permalink raw reply related
* [bpf-next PATCH 4/4] bpf: sockmap, more BPF_SK_SKB_STREAM_VERDICT tests
From: John Fastabend @ 2018-03-26 17:22 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
In-Reply-To: <20180326172022.20504.1790.stgit@john-Precision-Tower-5810>
Add BPF_SK_SKB_STREAM_VERDICT tests for ingress hook. While
we do this also bring stream tests in-line with MSG based
testing.
A map for skb options is added for userland to push options
at BPF programs.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
samples/sockmap/sockmap_kern.c | 21 ++++++++++++++++++---
samples/sockmap/sockmap_test.sh | 20 +++++++++++++++++++-
samples/sockmap/sockmap_user.c | 23 +++++++++++++++++++++++
3 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/samples/sockmap/sockmap_kern.c b/samples/sockmap/sockmap_kern.c
index ca28722..9ff8bc5 100644
--- a/samples/sockmap/sockmap_kern.c
+++ b/samples/sockmap/sockmap_kern.c
@@ -85,6 +85,12 @@ struct bpf_map_def SEC("maps") sock_redir_flags = {
.max_entries = 1
};
+struct bpf_map_def SEC("maps") sock_skb_opts = {
+ .type = BPF_MAP_TYPE_ARRAY,
+ .key_size = sizeof(int),
+ .value_size = sizeof(int),
+ .max_entries = 1
+};
SEC("sk_skb1")
int bpf_prog1(struct __sk_buff *skb)
@@ -97,15 +103,24 @@ int bpf_prog2(struct __sk_buff *skb)
{
__u32 lport = skb->local_port;
__u32 rport = skb->remote_port;
- int ret = 0;
+ int len, *f, ret, zero = 0;
+ __u64 flags = 0;
if (lport == 10000)
ret = 10;
else
ret = 1;
- bpf_printk("sockmap: %d -> %d @ %d\n", lport, bpf_ntohl(rport), ret);
- return bpf_sk_redirect_map(skb, &sock_map, ret, 0);
+ len = (__u32)skb->data_end - (__u32)skb->data;
+ f = bpf_map_lookup_elem(&sock_skb_opts, &zero);
+ if (f && *f) {
+ ret = 3;
+ flags = *f;
+ }
+
+ bpf_printk("sk_skb2: redirect(%iB) flags=%i\n",
+ len, flags);
+ return bpf_sk_redirect_map(skb, &sock_map, ret, flags);
}
SEC("sockops")
diff --git a/samples/sockmap/sockmap_test.sh b/samples/sockmap/sockmap_test.sh
index 13b205f..ace75f0 100755
--- a/samples/sockmap/sockmap_test.sh
+++ b/samples/sockmap/sockmap_test.sh
@@ -1,5 +1,5 @@
#Test a bunch of positive cases to verify basic functionality
-for prog in "--txmsg_redir --txmsg_ingress" "--txmsg" "--txmsg_redir" "--txmsg_redir --txmsg_ingress" "--txmsg_drop"; do
+for prog in "--txmsg_redir --txmsg_skb" "--txmsg_redir --txmsg_ingress" "--txmsg" "--txmsg_redir" "--txmsg_redir --txmsg_ingress" "--txmsg_drop"; do
for t in "sendmsg" "sendpage"; do
for r in 1 10 100; do
for i in 1 10 100; do
@@ -109,6 +109,15 @@ for t in "sendmsg" "sendpage"; do
sleep 2
done
+prog="--txmsg_redir --txmsg_apply 1 --txmsg_skb"
+
+for t in "sendmsg" "sendpage"; do
+ TEST="./sockmap --cgroup /mnt/cgroup2/ -t $t -r $r -i $i -l $l $prog"
+ echo $TEST
+ $TEST
+ sleep 2
+done
+
# Test apply and redirect with larger value than send
r=1
@@ -132,6 +141,15 @@ for t in "sendmsg" "sendpage"; do
sleep 2
done
+prog="--txmsg_redir --txmsg_apply 2048 --txmsg_skb"
+
+for t in "sendmsg" "sendpage"; do
+ TEST="./sockmap --cgroup /mnt/cgroup2/ -t $t -r $r -i $i -l $l $prog"
+ echo $TEST
+ $TEST
+ sleep 2
+done
+
# Test apply and redirect with apply that never reaches limit
r=1024
diff --git a/samples/sockmap/sockmap_user.c b/samples/sockmap/sockmap_user.c
index f7503f4..6f23349 100644
--- a/samples/sockmap/sockmap_user.c
+++ b/samples/sockmap/sockmap_user.c
@@ -65,6 +65,7 @@
int txmsg_start;
int txmsg_end;
int txmsg_ingress;
+int txmsg_skb;
static const struct option long_options[] = {
{"help", no_argument, NULL, 'h' },
@@ -85,6 +86,7 @@
{"txmsg_start", required_argument, NULL, 's'},
{"txmsg_end", required_argument, NULL, 'e'},
{"txmsg_ingress", no_argument, &txmsg_ingress, 1 },
+ {"txmsg_skb", no_argument, &txmsg_skb, 1 },
{0, 0, NULL, 0 }
};
@@ -828,6 +830,27 @@ int main(int argc, char **argv)
err, strerror(errno));
}
}
+
+ if (txmsg_skb) {
+ int skb_fd = (test == SENDMSG || test == SENDPAGE) ? p2 : p1;
+ int ingress = BPF_F_INGRESS;
+
+ i = 0;
+ err = bpf_map_update_elem(map_fd[7], &i, &ingress, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (txmsg_ingress): %d (%s)\n",
+ err, strerror(errno));
+ }
+
+ i = 3;
+ err = bpf_map_update_elem(map_fd[0], &i, &skb_fd, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (c1 sockmap): %d (%s)\n",
+ err, strerror(errno));
+ }
+ }
}
if (txmsg_drop)
^ permalink raw reply related
* Re: [PATCH v2 iproute2-next 3/6] rdma: Add CM_ID resource tracking information
From: Leon Romanovsky @ 2018-03-26 17:27 UTC (permalink / raw)
To: Steve Wise; +Cc: David Ahern, stephen, netdev, linux-rdma
In-Reply-To: <6c376ad8-7b42-8b13-1eba-328fecd0e3bd@opengridcomputing.com>
[-- Attachment #1: Type: text/plain, Size: 2644 bytes --]
On Mon, Mar 26, 2018 at 12:13:53PM -0500, Steve Wise wrote:
>
>
> On 3/26/2018 12:06 PM, Leon Romanovsky wrote:
> > On Mon, Mar 26, 2018 at 10:24:25AM -0500, Steve Wise wrote:
> >>
> >> On 3/26/2018 10:08 AM, Leon Romanovsky wrote:
> >>> On Mon, Mar 26, 2018 at 09:55:46AM -0500, Steve Wise wrote:
> >>>> On 3/26/2018 9:44 AM, David Ahern wrote:
> >>>>> On 3/26/18 8:30 AM, Steve Wise wrote:
> >>>>>> On 3/26/2018 9:17 AM, David Ahern wrote:
> >>>>>>> On 2/27/18 9:07 AM, Steve Wise wrote:
> >>>>>>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >>>>>>>> index 5809f70..e55205b 100644
> >>>>>>>> --- a/rdma/rdma.h
> >>>>>>>> +++ b/rdma/rdma.h
> >>>>>>>> @@ -18,10 +18,12 @@
> >>>>>>>> #include <libmnl/libmnl.h>
> >>>>>>>> #include <rdma/rdma_netlink.h>
> >>>>>>>> #include <time.h>
> >>>>>>>> +#include <net/if_arp.h>
> >>>>>>>>
> >>>>>>>> #include "list.h"
> >>>>>>>> #include "utils.h"
> >>>>>>>> #include "json_writer.h"
> >>>>>>>> +#include <rdma/rdma_cma.h>
> >>>>>>>>
> >>>>>>> did you forget to add rdma_cma.h? I don't see that file in my repo.
> >>>>>> It is provided by the rdma-core package, upon which rdma tool now
> >>>>>> depends for the rdma_port_space enum.
> >>>>>>
> >>>>> You need to add a check for the package, and only build rdma if that
> >>>>> package is installed. See check_mnl in configure for an example.
> >>>> Ok, that makes sense.
> >>> IMHO, better solution will be to copy those files to iproute2.
> >> Hey Leon,
> >>
> >> Why is it better in your opinion? My gut tells me adding rdma_cma.h to
> >> iproute2 means more uabi type syncing.
> > Making rdmatool be dependant on rdma-core will require that distributions
> > will update their specs to install rdma-core as a dependency for every
> > iprotue2 install.
> >
> > The rdma-core dependency makes sense for RDMA users, but doesn't for most of
> > the iproute2 users.
>
> I'm fuzzy on the details of distro packaging, but David's suggestion is
> that rdmatool wouldn't get built if rdma-core isn't present. But
> everything else would. Just like it does not get built if libmnl is not
> installed. For pre-built rpms, the rdma-core would have to be present.
>
> I'm ok pulling it in, I'm just trying to understand. :)
>
Distros supply pre-built packages, for example Fedora's RPM:
https://rpmfind.net/linux/RPM/fedora/27/x86_64/i/iproute-4.12.0-3.fc27.x86_64.html
It requires that libmnl will be installed. Once rdmatool will need
rdma-core, it will pulled in too.
BTW, don't forget to change header's guards (ifdef/defne ..), see
rdma_netlink.h as an example.
> Steve.
>
>
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [net PATCH v2] net: sched, fix OOO packets with pfifo_fast
From: Cong Wang @ 2018-03-26 17:30 UTC (permalink / raw)
To: John Fastabend
Cc: Eric Dumazet, Jiri Pirko, David Miller,
Linux Kernel Network Developers
In-Reply-To: <20180325052505.4098.36912.stgit@john-Precision-Tower-5810>
On Sat, Mar 24, 2018 at 10:25 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> After the qdisc lock was dropped in pfifo_fast we allow multiple
> enqueue threads and dequeue threads to run in parallel. On the
> enqueue side the skb bit ooo_okay is used to ensure all related
> skbs are enqueued in-order. On the dequeue side though there is
> no similar logic. What we observe is with fewer queues than CPUs
> it is possible to re-order packets when two instances of
> __qdisc_run() are running in parallel. Each thread will dequeue
> a skb and then whichever thread calls the ndo op first will
> be sent on the wire. This doesn't typically happen because
> qdisc_run() is usually triggered by the same core that did the
> enqueue. However, drivers will trigger __netif_schedule()
> when queues are transitioning from stopped to awake using the
> netif_tx_wake_* APIs. When this happens netif_schedule() calls
> qdisc_run() on the same CPU that did the netif_tx_wake_* which
> is usually done in the interrupt completion context. This CPU
> is selected with the irq affinity which is unrelated to the
> enqueue operations.
Interesting. Why this is unique to pfifo_fast? For me it could
happen to other qdisc's too, when we release the qdisc root
lock in sch_direct_xmit(), another CPU could dequeue from
the same qdisc and transmit the skb in parallel too?
...
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 7e3fbe9..39c144b 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -373,24 +373,33 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
> */
> static inline bool qdisc_restart(struct Qdisc *q, int *packets)
> {
> + bool more, validate, nolock = q->flags & TCQ_F_NOLOCK;
> spinlock_t *root_lock = NULL;
> struct netdev_queue *txq;
> struct net_device *dev;
> struct sk_buff *skb;
> - bool validate;
>
> /* Dequeue packet */
> + if (nolock && test_and_set_bit(__QDISC_STATE_RUNNING, &q->state))
> + return false;
> +
Nit: you probably want to move the comment below this if check,
or simply remove it since it is useless...
^ permalink raw reply
* Re: syzbot rcu/debugobjects warning
From: Guillaume Nault @ 2018-03-26 17:38 UTC (permalink / raw)
To: Joel Fernandes
Cc: Thomas Gleixner, Paul McKenney, LKML, Todd Poynor,
open list:BPF (Safe dynamic programs and tools), Ben Hutchings,
Greg Kroah-Hartman
In-Reply-To: <CAJWu+or1YJEZX8R1fwJrb=cUc05_uzwZHJhD5-SMEycUO7W1mQ@mail.gmail.com>
On Sat, Mar 24, 2018 at 11:29:42PM -0700, Joel Fernandes wrote:
> On Fri, Mar 23, 2018 at 1:41 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Fri, 23 Mar 2018, Joel Fernandes wrote:
> >> On Fri, Mar 23, 2018 at 2:11 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> > On Thu, 22 Mar 2018, Joel Fernandes wrote:
> >> Sorry. Here is the raw crash log: https://pastebin.com/raw/puvh0cXE
> >> (The kernel logs are toward the end with the above).
> >
> > And that is interesting:
> >
> > [ 150.629667] <IRQ> [ 150.631700] [<ffffffff81d96069>] dump_stack+0xc1/0x128
> > [ 150.637051] [<ffffffff81dfefb6>] ? __debug_object_init+0x526/0xc40
> > [ 150.643431] [<ffffffff8142fbd1>] panic+0x1bc/0x3a8
> > [ 150.648416] [<ffffffff8142fa15>] ? percpu_up_read_preempt_enable.constprop.53+0xd7/0xd7
> > [ 150.656611] [<ffffffff81430835>] ? load_image_and_restore+0xf9/0xf9
> > [ 150.663070] [<ffffffff81269efd>] ? vprintk_default+0x1d/0x30
> > [ 150.668925] [<ffffffff81131879>] ? __warn+0x1a9/0x1e0
> > [ 150.674170] [<ffffffff81dfefb6>] ? __debug_object_init+0x526/0xc40
> > [ 150.680543] [<ffffffff81131894>] __warn+0x1c4/0x1e0
> > [ 150.685614] [<ffffffff81131afc>] warn_slowpath_null+0x2c/0x40
> > [ 150.691972] [<ffffffff81dfefb6>] __debug_object_init+0x526/0xc40
> > [ 150.698174] [<ffffffff81dfea90>] ? debug_object_fixup+0x30/0x30
> > [ 150.704283] [<ffffffff81dff709>] debug_object_init_on_stack+0x19/0x20
> > [ 150.710917] [<ffffffff81287a93>] __wait_rcu_gp+0x93/0x1b0
> > [ 150.716508] [<ffffffff81290251>] synchronize_rcu.part.65+0x101/0x110
> > [ 150.723054] [<ffffffff81290150>] ? rcu_pm_notify+0xc0/0xc0
> > [ 150.728735] [<ffffffff81292bc0>] ? __call_rcu.constprop.72+0x910/0x910
> > [ 150.735459] [<ffffffff81235221>] ? __lock_is_held+0xa1/0xf0
> > [ 150.741223] [<ffffffff81290287>] synchronize_rcu+0x27/0x90
> >
> > So this calls synchronize_rcu from a rcu callback. That's a nono. This is
> > on the back of an interrupt in softirq context and __wait_rcu_gp() can
> > sleep, which is obviously a bad idea in softirq context....
> >
> > Cc'ed netdev ....
> >
> > And that also explains the debug object splat because this is not running
> > on the task stack. It's running on the softirq stack ....
> >
> > [ 150.746908] [<ffffffff83588b35>] __l2tp_session_unhash+0x3d5/0x550
> > [ 150.753281] [<ffffffff8358891f>] ? __l2tp_session_unhash+0x1bf/0x550
> > [ 150.759828] [<ffffffff8114596a>] ? __local_bh_enable_ip+0x6a/0xd0
> > [ 150.766123] [<ffffffff8358ddb0>] ? l2tp_udp_encap_recv+0xd90/0xd90
> > [ 150.772497] [<ffffffff83588e97>] l2tp_tunnel_closeall+0x1e7/0x3a0
> > [ 150.778782] [<ffffffff835897be>] l2tp_tunnel_destruct+0x30e/0x5a0
> > [ 150.785067] [<ffffffff8358965a>] ? l2tp_tunnel_destruct+0x1aa/0x5a0
> > [ 150.791537] [<ffffffff835894b0>] ? l2tp_tunnel_del_work+0x460/0x460
> > [ 150.797997] [<ffffffff82ee8053>] __sk_destruct+0x53/0x570
> > [ 150.803588] [<ffffffff81293918>] rcu_process_callbacks+0x898/0x1300
> > [ 150.810048] [<ffffffff812939f7>] ? rcu_process_callbacks+0x977/0x1300
> > [ 150.816684] [<ffffffff82ee8000>] ? __sk_dst_check+0x240/0x240
> > [ 150.822625] [<ffffffff838be5d6>] __do_softirq+0x206/0x951
> > [ 150.828223] [<ffffffff81147315>] irq_exit+0x165/0x190
> > [ 150.833557] [<ffffffff838bd1eb>] smp_apic_timer_interrupt+0x7b/0xa0
> > [ 150.840018] [<ffffffff838b9470>] apic_timer_interrupt+0xa0/0xb0
> > [ 150.846132] <EOI> [ 150.848166] [<ffffffff838b6756>] ? native_safe_halt+0x6/0x10
> > [ 150.854036] [<ffffffff8123bf2d>] ? trace_hardirqs_on+0xd/0x10
> > [ 150.859973] [<ffffffff838b5d85>] default_idle+0x55/0x360
> > [ 150.865478] [<ffffffff8106be0a>] arch_cpu_idle+0xa/0x10
> > [ 150.870896] [<ffffffff838b6b96>] default_idle_call+0x36/0x60
> > [ 150.876751] [<ffffffff81226cb0>] cpu_startup_entry+0x2b0/0x380
> > [ 150.882787] [<ffffffff81226a00>] ? cpu_in_idle+0x20/0x20
> > [ 150.888291] [<ffffffff812d2343>] ? clockevents_register_device+0x123/0x200
> > [ 150.895358] [<ffffffff810b0693>] start_secondary+0x303/0x3e0
> > [ 150.901209] [<ffffffff810b0390>] ? set_cpu_sibling_map+0x11f0/0x11f0
>
> Thomas, thanks a lot. It appears this issue will not happen on
> mainline since from commit 765924e362d1 (subject "l2tp: don't close
> sessions in l2tp_tunnel_destruct()"), l2tp_tunnel_closeall is no
> longer called from l2tp_tunnel_destruct. From that commit message it
> seems one of the motivations is to solve scheduling from atomic issue.
>
I agree that this patch should fix the above splat.
> However for this change to be applied to android-4.9 and/or 4.9
> stable, it depends on several other l2p patches and they aren't
> straight forward cherry-picks from mainline (and I don't have much
> background with this driver).
>
> v3.16.56 stable seems to be further along with l2tp than v4.9.89, in
> that it atleast has more of the upstream patches adapted for it, that
> the above patch depends on. Since this also related to stable, I am
> CC'ing Greg kh and Ben.
>
I generally review l2tp patches proposed for -stable trees (although
not in time). If a patch has been ported to 3.16.y and is missing in
another tree, then it should be safe to port it there too.
> Here are some of the commits in 3.16 stable that I couldn't find
> applied to v4.9 stable. The above fix quotes the below patches as
> dependencies so they would need to be stable backported. Also CC'ing
> Guillaume since he authored the above mentioned fix.
>
> 0c15ddabbcf l2tp: don't register sessions in l2tp_session_create()
> a3c5d5b70f4e l2tp: fix race condition in l2tp_tunnel_delete
> 5b216e8dcda2 l2tp: prevent creation of sessions on terminated tunnels
> 76ff5e22f1e0 l2tp: hold tunnel while looking up sessions in l2tp_netlink
> ceb8f6b23a38 l2tp: define parameters of l2tp_session_get*() as "const"
> 0295d020b63f l2tp: initialise session's refcount before making it reachable
> 29a77518927e l2tp: take reference on sessions being dumped
> b301c9b7782f l2tp: take a reference on sessions used in genetlink handlers
>
Yes, I think it'd make sense to port these patches.
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 17:55 UTC (permalink / raw)
To: Mathieu Desnoyers, rostedt
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <1218234422.259.1522083422808.JavaMail.zimbra@efficios.com>
On 3/26/18 9:57 AM, Mathieu Desnoyers wrote:
> ----- On Mar 26, 2018, at 12:35 PM, rostedt rostedt@goodmis.org wrote:
>
>> On Mon, 26 Mar 2018 09:25:07 -0700
>> Alexei Starovoitov <ast@fb.com> wrote:
>>
>>> commit log of patch 6 states:
>>>
>>> "for_each_tracepoint_range() api has no users inside the kernel.
>>> Make it more useful with ability to stop for_each() loop depending
>>> via callback return value.
>>> In such form it's used in subsequent patch."
>>>
>>> and in patch 7:
>>>
>>> +static void *__find_tp(struct tracepoint *tp, void *priv)
>>> +{
>>> + char *name = priv;
>>> +
>>> + if (!strcmp(tp->name, name))
>>> + return tp;
>>> + return NULL;
>>> +}
>>> ...
>>> + struct tracepoint *tp;
>>> ...
>>> + tp = for_each_kernel_tracepoint(__find_tp, tp_name);
>>> + if (!tp)
>>> + return -ENOENT;
>>>
>>> still not obvious?
>>
>> Please just create a new function called tracepoint_find_by_name(), and
>> use that. I don't see any benefit in using a for_each* function for
>> such a simple routine. Not to mention, you then don't need to know the
>> internals of a tracepoint in kernel/bpf/syscall.c.
>
> Steven's approach is fine by me, considering there should never be duplicated
> tracepoint definitions (it emits a __tracepoint_##name symbol which would cause
> multiple symbols definition errors at link time if there are more than
> a single definition per tracepoint name in the core kernel). The exported
> API should probably be named "kernel_tracepoint_find_by_name()" or something
> similar, thus indicating that it only lookup tracepoints in the core kernel.
An email ago you were ok to s/return/return NULL/ in your out-of-tree
module, but now flip flop to add new function approach just to
reduce the work you need to do in lttng?
We're not talking about changing __kmalloc signature here.
My patch extends for_each_kernel_tracepoint() api similar to other
for_each_*() iterators and improves possible uses of it.
It can event help lltng.
lttng-tracepoint.c is doing:
for_each_kernel_tracepoint(lttng_kernel_tracepoint_add, &ret);
to copy kernel tracepoints into its own hash table.
Only to later do:
/*
* Get tracepoint if the tracepoint is present in the tracepoint hash
table.
* Must be called with lttng_tracepoint_mutex held.
* Returns NULL if not present.
*/
static
struct tracepoint_entry *get_tracepoint(const char *name)
{
struct hlist_head *head;
struct tracepoint_entry *e;
u32 hash = jhash(name, strlen(name), 0);
head = &tracepoint_table[hash & (TRACEPOINT_TABLE_SIZE - 1)];
lttng_hlist_for_each_entry(e, head, hlist) {
if (!strcmp(name, e->name))
return e;
}
return NULL;
}
this use case potentially can be reimplemented in lttng with
this extended for_each_kernel_tracepoint() api.
Like I do on bpf side with very similar callback:
+static void *__find_tp(struct tracepoint *tp, void *priv)
+{
+ char *name = priv;
+
+ if (!strcmp(tp->name, name))
+ return tp;
+ return NULL;
+}
> Which brings the next question: what are Alexei's plan to handle tracepoints
> in modules, considering module load/unload scenarios ? The tracepoint API
> has module notifiers for this, but it does not appear to be used in this
> patch series.
correct. this set deals with in-kernel tracepoints only.
No attempt to do anything with tracepoints inside modules.
but your question brings another question:
why kernel/module.c have this code:
mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs"
sizeof(*mod->tracepoints_ptrs),
&mod->num_tracepoints);
and the whole module tracepoint notifier logic
that is only used by out-of-tree ?
I didn't realize so much of kernel code was taken hostage by lttng.
One thing is to be nice to out-of-tree and do not break them
for no reason, but arguing that kernel shouldn't add a minor extension
to for_each_kernel_tracepoint() api is really taking the whole thing
to next level.
Also I hope you noticed that the patch is doing:
+++ b/include/linux/tracepoint-defs.h
@@ -33,6 +33,7 @@ struct tracepoint {
int (*regfunc)(void);
void (*unregfunc)(void);
struct tracepoint_func __rcu *funcs;
+ u32 num_args;
};
To make sure that bpf programs are safe I need to do a static check
in the verifier that programs don't access arguments beyond
those specified by the tracepoint.
That was mentioned in the commit log of patch 6 too:
"
compute number of arguments passed into tracepoint
at compile time and store it as part of 'struct tracepoint'.
The number is necessary to check safety of bpf program access that
is coming in subsequent patch.
"
I suspect you will have the same objection?
It breaks out-of-tree modules?!
^ permalink raw reply
* Re: NETDEV WATCHDOG: eth0 (tg3): transmit queue 0 timed out
From: Borislav Petkov @ 2018-03-26 18:05 UTC (permalink / raw)
To: Satish Baddipadige
Cc: Siva Reddy Kallam, Linux Netdev List, Prashant Sreedharan,
Michael Chan, Linux Kernel list
In-Reply-To: <CA+oDAx8CCW8rUfdqSZJpKFzHUBocY_Q1xbJXB0y8qa6qP0iUAg@mail.gmail.com>
On Tue, Mar 20, 2018 at 11:41:06AM +0530, Satish Baddipadige wrote:
> Can you please test the attached patch?
Well, the network connection just died with it. It didn't fire the
netdev watchdog but I still had to down and up eth0 in order to continue
using it. ssh connection into the box survived so I didn't have to login
again but it still died intermittently.
I'll keep playing with it to see if I'll catch some sort of splat...
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH] Documentation/isdn: check and fix dead links ...
From: Paul Bolle @ 2018-03-26 18:05 UTC (permalink / raw)
To: Sanjeev Gupta; +Cc: David Miller, corbet, isdn, netdev
In-Reply-To: <20180326.123125.1782554768511538474.davem@davemloft.net>
Sanjeev,
On Mon, 2018-03-26 at 12:31 -0400, David Miller wrote:
> > and switch to https where possible.
> >
> > All links have been eyeballed to verify that the domains have
> > not changed, etc.
> >
> > Signed-off-by: Sanjeev Gupta <ghane0@gmail.com>
>
> Applied, thank you.
Your patch never hit my inbox. The joy of email! Anyhow, the diff on spinic's
netdev archive suggests you left (at least) one reference to http://gigaset307
x.sourceforge.net/ in the documentation.
If you'd be so kind to send a follow up patch to clean up that reference too
that would be appreciated.
Thanks!
Paul Bolle
^ permalink raw reply
* Re: [PATCH] Documentation/isdn: check and fix dead links ...
From: Paul Bolle @ 2018-03-26 18:10 UTC (permalink / raw)
To: Sanjeev Gupta; +Cc: David Miller, corbet, isdn, netdev
In-Reply-To: <1522087557.2883.12.camel@tiscali.nl>
On Mon, 2018-03-26 at 20:05 +0200, Paul Bolle wrote:
> Your patch never hit my inbox. The joy of email! Anyhow, the diff on spinic's
> netdev archive suggests you left (at least) one reference to http://gigaset307
> x.sourceforge.net/ in the documentation.
>
> If you'd be so kind to send a follow up patch to clean up that reference too
> that would be appreciated.
Never mind. That doesn't actually work over https.
Meh!
Paul Bolle
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 18:11 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <17073efa-d833-7348-bef1-79376ad43bc6@fb.com>
On Mon, 26 Mar 2018 10:55:51 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> An email ago you were ok to s/return/return NULL/ in your out-of-tree
> module, but now flip flop to add new function approach just to
> reduce the work you need to do in lttng?
> We're not talking about changing __kmalloc signature here.
> My patch extends for_each_kernel_tracepoint() api similar to other
> for_each_*() iterators and improves possible uses of it.
Alexei, do you have another use case for using
for_each_kernel_tracepoint() other than the find_tp? If so, then I'm
sure Mathieu can handle the change.
But I think it's cleaner to add a tracepoint_find_by_name() function.
If you come up with another use case for using the for_each* function
then we'll consider changing it then.
> One thing is to be nice to out-of-tree and do not break them
> for no reason, but arguing that kernel shouldn't add a minor extension
> to for_each_kernel_tracepoint() api is really taking the whole thing
> to next level.
That's not the point. I disagree with the reason for the change, and
believe that it would be cleaner to add a find_by_name() function.
Which would make your patch set even cleaner.
Instead of having in the bpf code:
static void *__find_tp(struct tracepoint *tp, void *priv)
{
char *name = priv;
if (!strcmp(tp->name, name))
return tp;
return NULL;
}
[..]
tp = for_each_kernel_tracepoint(__find_tp, tp_name);
if (!tp)
return -ENOENT;
You would simply have:
tp = tracepoint_find_by_name(tp_name);
if (!tp)
return -ENOENT;
That would make the code more obvious to what it is doing. And this
does not impede your patch set at all.
-- Steve
^ permalink raw reply
* Re: [net PATCH v2] net: sched, fix OOO packets with pfifo_fast
From: John Fastabend @ 2018-03-26 18:16 UTC (permalink / raw)
To: Cong Wang
Cc: Eric Dumazet, Jiri Pirko, David Miller,
Linux Kernel Network Developers
In-Reply-To: <CAM_iQpWNX-9p-bo+caUyJ8yfsNDS1a2pV9LNvHK4=y3ec4qRVw@mail.gmail.com>
On 03/26/2018 10:30 AM, Cong Wang wrote:
> On Sat, Mar 24, 2018 at 10:25 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>> After the qdisc lock was dropped in pfifo_fast we allow multiple
>> enqueue threads and dequeue threads to run in parallel. On the
>> enqueue side the skb bit ooo_okay is used to ensure all related
>> skbs are enqueued in-order. On the dequeue side though there is
>> no similar logic. What we observe is with fewer queues than CPUs
>> it is possible to re-order packets when two instances of
>> __qdisc_run() are running in parallel. Each thread will dequeue
>> a skb and then whichever thread calls the ndo op first will
>> be sent on the wire. This doesn't typically happen because
>> qdisc_run() is usually triggered by the same core that did the
>> enqueue. However, drivers will trigger __netif_schedule()
>> when queues are transitioning from stopped to awake using the
>> netif_tx_wake_* APIs. When this happens netif_schedule() calls
>> qdisc_run() on the same CPU that did the netif_tx_wake_* which
>> is usually done in the interrupt completion context. This CPU
>> is selected with the irq affinity which is unrelated to the
>> enqueue operations.
>
> Interesting. Why this is unique to pfifo_fast? For me it could
> happen to other qdisc's too, when we release the qdisc root
> lock in sch_direct_xmit(), another CPU could dequeue from
> the same qdisc and transmit the skb in parallel too?
>
Agreed, my guess is it never happens because the timing is
tighter in the lock case. Or if it is happening its infrequent
enough that no one noticed the OOO packets.
For net-next we probably could clean this up. I was just
going for something simple in net that didn't penalize all
qdiscs as Eric noted. This patch doesn't make it any worse
at least. And we have been living with the above race for
years.
> ...
>
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index 7e3fbe9..39c144b 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -373,24 +373,33 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>> */
>> static inline bool qdisc_restart(struct Qdisc *q, int *packets)
>> {
>> + bool more, validate, nolock = q->flags & TCQ_F_NOLOCK;
>> spinlock_t *root_lock = NULL;
>> struct netdev_queue *txq;
>> struct net_device *dev;
>> struct sk_buff *skb;
>> - bool validate;
>>
>> /* Dequeue packet */
>> + if (nolock && test_and_set_bit(__QDISC_STATE_RUNNING, &q->state))
>> + return false;
>> +
>
> Nit: you probably want to move the comment below this if check,
> or simply remove it since it is useless...
>
hmm I was planning to do a comment rewrite patch to bring the comments
in sch_generic.c up to date in net-next I'll delete it there. I think
we can live with the extra line in net. Also Eric pointed out that
qdisc_restart is not really a good name anymore for this routine.
.John
^ permalink raw reply
* Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper
From: Mike Looijmans @ 2018-03-26 18:21 UTC (permalink / raw)
To: Florian Fainelli, David Miller
Cc: netdev, linux-kernel, devicetree, andrew, robh+dt, frowand.list
In-Reply-To: <40e5302c-0c00-dba0-58d3-77407aa6bde8@gmail.com>
On 26-03-18 19:05, Florian Fainelli wrote:
> On 03/26/2018 09:58 AM, David Miller wrote:
>> From: Mike Looijmans <mike.looijmans@topic.nl>
>> Date: Mon, 26 Mar 2018 08:41:29 +0200
>>
>>> It's common practice to store MAC addresses for network interfaces into
>>> nvmem devices. However the code to actually do this in the kernel lacks,
>>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>>> address from an nvmem cell provider.
>>>
>>> This is particulary useful on devices where the ethernet interface cannot
>>> be configured by the bootloader, for example because it's in an FPGA.
>>>
>>> Tested by adapting the cadence macb driver to call this instead of
>>> of_get_mac_address().
>>>
>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>> ---
>>> v2: Use of_nvmem_cell_get to avoid needing the assiciated device
>>> Use void* instead of char*
>>> Add devicetree binding doc
>>
>> Like Andrew, I think you should add a new interface for getting the MAC
>> address from nvmem.
>>
>> And drivers can call both of them if they want OF device tree and
>> NVMEM probing for MAC addresses.
>>
>> Later you can add a consolidated interface, if necessary, which does
>> both and also take a reference to the MAC address buffer of the driver
>> in order to deal with the resource allocation issues.
>
> Agreed, also, how does this fit with Alban's patch series here:
Ok, makes sense. I'll cook up a v3.
>
> https://lkml.org/lkml/2018/3/24/312
>
> do you depend on those changes at all?
>
As far as I can tell, there's no dependency, I'm adding an nvmem
"consumer" while Alban's patch adds a "provider". I'm mainly interested
in storing the MAC address into an I2C EEPROM, which you can also buy
with a pre-programmed MAC address in the first 6 bytes, so there's no
production cost for managing that.
Alban's patch might some day collide with my idea of making hardware
that has some unique ID into nvmem providers, like 1-wire chips, some
NOR flash chips (there's the conflict) and various other devices. That
would allow a board to obtain a random yet constant MAC address without
any additional hardware. I'll cross that bridge when I find it.
--
Mike Looijmans
^ permalink raw reply
* Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper
From: Mike Looijmans @ 2018-03-26 18:25 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, linux-kernel, devicetree, f.fainelli, robh+dt,
frowand.list
In-Reply-To: <20180326155010.GC3014@lunn.ch>
On 26-03-18 17:50, Andrew Lunn wrote:
> On Mon, Mar 26, 2018 at 08:41:29AM +0200, Mike Looijmans wrote:
>> It's common practice to store MAC addresses for network interfaces into
>> nvmem devices. However the code to actually do this in the kernel lacks,
>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>> address from an nvmem cell provider.
>>
>> This is particulary useful on devices where the ethernet interface cannot
>> be configured by the bootloader, for example because it's in an FPGA.
>>
>> Tested by adapting the cadence macb driver to call this instead of
>> of_get_mac_address().
>
> Hi Mike
>
> I can understand you not wanting to modify all the call sites for
> of_get_mac_address().
>
> However, the name of_get_nvmem_mac_address() suggests it gets the MAC
> address from NVMEM. I think people are going to be surprised when they
> find it first tries for a MAC address directly in device tree. I would
> drop the call to of_get_mac_address(), and have the MAC driver call
> both.
>
> You could also maybe take a look at fwnode_get_mac_address(). It
> should work for both OF and ACPI. It fits better because is passes a
> char * for the address. You could make that do both, and call it from
> the macb driver. dev_fwnode() probably does what you want.
fwnode_get_mac_address looks really new, there's only one user so far.
Is it the intention that all drivers eventually migrate to that?
(It also means I cannot backport it to the kernel I'm actually using,
'cause I haven't got the Zynq to work with the mainline macb driver yet.
But that's just my problem...)
--
Mike Looijmans
^ permalink raw reply
* Re: [PATCH net-next nfs 1/6] net: Convert rpcsec_gss_net_ops
From: J. Bruce Fields @ 2018-03-26 18:36 UTC (permalink / raw)
To: Anna Schumaker
Cc: Kirill Tkhai, davem, trond.myklebust, jlayton, dhowells, keescook,
dwindsor, ishkamiel, elena.reshetova, linux-nfs, linux-afs,
netdev
In-Reply-To: <28c32fa9-17e1-0399-d7bf-760441128cf8@Netapp.com>
On Fri, Mar 23, 2018 at 02:53:34PM -0400, Anna Schumaker wrote:
>
>
> On 03/13/2018 06:49 AM, Kirill Tkhai wrote:
> > These pernet_operations initialize and destroy sunrpc_net_id refered
> > per-net items. Only used global list is cache_list, and accesses
> > already serialized.
> >
> > sunrpc_destroy_cache_detail() check for list_empty() without
> > cache_list_lock, but when it's called from
> > unregister_pernet_subsys(), there can't be callers in parallel, so
> > we won't miss list_empty() in this case.
> >
> > Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>
> It might make sense to take these and the other NFS patches through
> the net tree, since the pernet_operations don't yet have the async
> field in my tree (and I therefore can't compile once these are
> applied).
Ditto for the nfsd patch, so, for what it's worth:
Acked-by: J. Bruce Fields <bfields@redhat.com>
for that patch.--b.
--b.
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 18:39 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <20180326141104.6d63660a@gandalf.local.home>
On 3/26/18 11:11 AM, Steven Rostedt wrote:
> On Mon, 26 Mar 2018 10:55:51 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>> An email ago you were ok to s/return/return NULL/ in your out-of-tree
>> module, but now flip flop to add new function approach just to
>> reduce the work you need to do in lttng?
>> We're not talking about changing __kmalloc signature here.
>> My patch extends for_each_kernel_tracepoint() api similar to other
>> for_each_*() iterators and improves possible uses of it.
>
> Alexei, do you have another use case for using
> for_each_kernel_tracepoint() other than the find_tp? If so, then I'm
> sure Mathieu can handle the change.
>
> But I think it's cleaner to add a tracepoint_find_by_name() function.
> If you come up with another use case for using the for_each* function
> then we'll consider changing it then.
another use case ?! Frankly such reasoning smells.
I'm fine doing quick followup patch to add tracepoint_find_by_name()
and restore 'return void' behavior of for_each_kernel_tracepoint's
callback, but I'm struggling to accept the precedent it will create
that all exported functions of kernel/tracepoint.c are really
lttng extensions and we cannot easily change them.
I'd like to hear Linus take on this.
^ permalink raw reply
* Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper
From: Andrew Lunn @ 2018-03-26 18:41 UTC (permalink / raw)
To: Mike Looijmans
Cc: netdev, linux-kernel, devicetree, f.fainelli, robh+dt,
frowand.list
In-Reply-To: <e151482e-7f67-c072-e946-95f6819c3bc2@topic.nl>
> fwnode_get_mac_address looks really new, there's only one user so far. Is it
> the intention that all drivers eventually migrate to that?
Hi Mike
Probably not. But any driver which needs to work with both ACPI and OF
is likely to use this API. So server class ARM64 chips for example.
Andrew
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 18:48 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <543f953a-8284-68f0-6a22-ec2d5f247199@fb.com>
On Mon, 26 Mar 2018 11:39:05 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> On 3/26/18 11:11 AM, Steven Rostedt wrote:
> > On Mon, 26 Mar 2018 10:55:51 -0700
> > Alexei Starovoitov <ast@fb.com> wrote:
> >
> >> An email ago you were ok to s/return/return NULL/ in your out-of-tree
> >> module, but now flip flop to add new function approach just to
> >> reduce the work you need to do in lttng?
> >> We're not talking about changing __kmalloc signature here.
> >> My patch extends for_each_kernel_tracepoint() api similar to other
> >> for_each_*() iterators and improves possible uses of it.
> >
> > Alexei, do you have another use case for using
> > for_each_kernel_tracepoint() other than the find_tp? If so, then I'm
> > sure Mathieu can handle the change.
> >
> > But I think it's cleaner to add a tracepoint_find_by_name() function.
> > If you come up with another use case for using the for_each* function
> > then we'll consider changing it then.
>
> another use case ?! Frankly such reasoning smells.
WTF is the big deal here?
>
> I'm fine doing quick followup patch to add tracepoint_find_by_name()
And BTW, it would actually have to be called
tracepoint_core_find_by_name() as it will not deal with modules.
Modules would have to have much more work to deal with.
> and restore 'return void' behavior of for_each_kernel_tracepoint's
What? you can't rebase now? Just don't touch that function.
> callback, but I'm struggling to accept the precedent it will create
> that all exported functions of kernel/tracepoint.c are really
> lttng extensions and we cannot easily change them.
First, my argument about your use case has little to do with LTTng.
I have to maintain this code, and this is my preference. Just like I do
the silly
/* Comment like this,
* for multiple lines
*/
When I deal with the networking code. Because that's the preference for
the networking folks.
> I'd like to hear Linus take on this.
I doubt he cares about something this petty.
-- Steve
^ permalink raw reply
* [PATCH net 1/1] net sched actions: fix dumping which requires several messages to user space
From: Craig Dillabaugh @ 2018-03-26 18:58 UTC (permalink / raw)
To: davem; +Cc: netdev, kernel, jhs, xiyou.wangcong, jiri, Craig Dillabaugh
Fixes a bug in the tcf_dump_walker function that can cause some actions
to not be reported when dumping a large number of actions. This issue
became more aggrevated when cookies feature was added. In particular
this issue is manifest when large cookie values are assigned to the
actions and when enough actions are created that the resulting table
must be dumped in multiple batches.
The number of actions returned in each batch is limited by the total
number of actions and the memory buffer size. With small cookies
the numeric limit is reached before the buffer size limit, which avoids
the code path triggering this bug. When large cookies are used buffer
fills before the numeric limit, and the erroneous code path is hit.
For example after creating 32 csum actions with the cookie
aaaabbbbccccdddd
$ tc actions ls action csum
total acts 26
action order 0: csum (tcp) action continue
index 1 ref 1 bind 0
cookie aaaabbbbccccdddd
.....
action order 25: csum (tcp) action continue
index 26 ref 1 bind 0
cookie aaaabbbbccccdddd
total acts 6
action order 0: csum (tcp) action continue
index 28 ref 1 bind 0
cookie aaaabbbbccccdddd
......
action order 5: csum (tcp) action continue
index 32 ref 1 bind 0
cookie aaaabbbbccccdddd
Note that the action with index 27 is omitted from the report.
Fixes: 4b3550ef530c ("[NET_SCHED]: Use nla_nest_start/nla_nest_end")"
Signed-off-by: Craig Dillabaugh <cdillaba@mojatatu.com>
---
net/sched/act_api.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index eba6682..efc6bfb 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -135,8 +135,10 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
continue;
nest = nla_nest_start(skb, n_i);
- if (!nest)
+ if (!nest) {
+ index--;
goto nla_put_failure;
+ }
err = tcf_action_dump_1(skb, p, 0, 0);
if (err < 0) {
index--;
--
1.9.1
^ permalink raw reply related
* Re: [RFC PATCH V2 0/8] Packed ring for vhost
From: Konrad Rzeszutek Wilk @ 2018-03-26 19:01 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, virtualization, kvm, netdev, linux-kernel
In-Reply-To: <1522035533-11786-1-git-send-email-jasowang@redhat.com>
On Mon, Mar 26, 2018 at 11:38:45AM +0800, Jason Wang wrote:
> Hi all:
>
> This RFC implement packed ring layout. The code were tested with pmd
> implement by Jens at
> http://dpdk.org/ml/archives/dev/2018-January/089417.html. Minor change
> was needed for pmd codes to kick virtqueue since it assumes a busy
> polling backend.
>
> Test were done between localhost and guest. Testpmd (rxonly) in guest
> reports 2.4Mpps. Testpmd (txonly) repots about 2.1Mpps.
And how does it compare to older ring layout?
>
> Notes: The event suppression /indirect descriptor support is complied
> test only because of lacked driver support.
>
> Changes from V1:
>
> - Refactor vhost used elem code to avoid open coding on used elem
> - Event suppression support (compile test only).
> - Indirect descriptor support (compile test only).
> - Zerocopy support.
> - vIOMMU support.
> - SCSI/VSOCK support (compile test only).
> - Fix several bugs
>
> For simplicity, I don't implement batching or other optimizations.
>
> Please review.
>
> Thanks
>
> Jason Wang (8):
> vhost: move get_rx_bufs to vhost.c
> vhost: hide used ring layout from device
> vhost: do not use vring_used_elem
> vhost_net: do not explicitly manipulate vhost_used_elem
> vhost: vhost_put_user() can accept metadata type
> virtio: introduce packed ring defines
> vhost: packed ring support
> vhost: event suppression for packed ring
>
> drivers/vhost/net.c | 138 ++-----
> drivers/vhost/scsi.c | 62 +--
> drivers/vhost/vhost.c | 818 ++++++++++++++++++++++++++++++++++---
> drivers/vhost/vhost.h | 46 ++-
> drivers/vhost/vsock.c | 42 +-
> include/uapi/linux/virtio_config.h | 9 +
> include/uapi/linux/virtio_ring.h | 32 ++
> 7 files changed, 921 insertions(+), 226 deletions(-)
>
> --
> 2.7.4
>
^ permalink raw reply
* Re: [bpf-next V5 PATCH 02/15] xdp: introduce xdp_return_frame API and use in cpumap
From: Jesper Dangaard Brouer @ 2018-03-26 19:06 UTC (permalink / raw)
To: Alexander Duyck
Cc: Netdev, BjörnTöpel, Karlsson, Magnus, Eugenia Emantayev,
Jason Wang, John Fastabend, Eran Ben Elisha, Saeed Mahameed,
Gal Pressman, Daniel Borkmann, Alexei Starovoitov, Tariq Toukan,
brouer
In-Reply-To: <CAKgT0UeZRHWOW4gsQD7OGWAD99ObHEqSUkvEdZjDG-iUY3wpSg@mail.gmail.com>
On Fri, 23 Mar 2018 09:35:47 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:
> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index b2362ddfa694..15b546325e31 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -33,16 +33,44 @@
> > * also mandatory during RX-ring setup.
> > */
> >
> > +enum mem_type {
> > + MEM_TYPE_PAGE_SHARED = 0, /* Split-page refcnt based model */
> > + MEM_TYPE_PAGE_ORDER0, /* Orig XDP full page model */
> > + MEM_TYPE_MAX,
> > +};
> > +
> > +struct xdp_mem_info {
> > + u32 type; /* enum mem_type, but known size type */
>
> Do you really need to make t his a full u32 value for something that
> is likely never going to exceed a single digit value?
Could we please save these kind of optimizations for later, please?
I do have a plan for state compression later. If you noticed, later
I'll add an 'id' member. And in the ID patch, I'm actually
limiting the IDs to max 16-bit. Thus, the plan is to allow for storing
type+id in e.g. a u32. BUT I see no reason do this now and complicate
the patchset further. (When we do this state compression, I actually
want to run some perf tests, that kind of work can cause unfortunate
compiler and assembler level constructs what are slower...)
> > + /* u32 id; will be added later in this patchset */
>
> Wouldn't it be better to just hold off and add it then instead of
> adding it as a comment?
I was trying to help the reviewer see where this was going. The line
will be updated within the patchset, like the comment promise, so there
is not real danger of having it here.
[...]
> > +static inline
> > +void xdp_return_frame(void *data, struct xdp_mem_info *mem)
> > +{
> > + if (mem->type == MEM_TYPE_PAGE_SHARED)
> > + page_frag_free(data);
> > +
> > + if (mem->type == MEM_TYPE_PAGE_ORDER0) {
> > + struct page *page = virt_to_page(data); /* Assumes order0 page*/
> > +
> > + put_page(page);
> > + }
>
> Actually page_frag_free would probably work for either one. Also is it
> safe to assume that the page is order 0? Are the only users of
> compound pages that support XDP also only supporting the page fragment
> setup?
>
> Also you probably don't need put_page. It might be better to use
> __free_page if you are certain the pages are coming from the Rx path
> of drivers and don't have any special destructors associated with
> them.
Can we also keep these kind of optimization for later, please? (p.s. I
do know this, e.g. I put a comment in page_pool of using __free_pages()).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ 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