Netdev List
 help / color / mirror / Atom feed
* [PATCH net 2/3] net: sched: multiq: don't call qdisc_put() while holding tree lock
From: Vlad Buslov @ 2019-09-18  7:32 UTC (permalink / raw)
  To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
In-Reply-To: <20190918073201.2320-1-vladbu@mellanox.com>

Recent changes that removed rtnl dependency from rules update path of tc
also made tcf_block_put() function sleeping. This function is called from
ops->destroy() of several Qdisc implementations, which in turn is called by
qdisc_put(). Some Qdiscs call qdisc_put() while holding sch tree spinlock,
which results sleeping-while-atomic BUG.

Steps to reproduce for multiq:

tc qdisc add dev ens1f0 root handle 1: multiq
tc qdisc add dev ens1f0 parent 1:10 handle 50: sfq perturb 10
ethtool -L ens1f0 combined 2
tc qdisc change dev ens1f0 root handle 1: multiq

Resulting dmesg:

[ 5539.419344] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:909
[ 5539.420945] in_atomic(): 1, irqs_disabled(): 0, pid: 27658, name: tc
[ 5539.422435] INFO: lockdep is turned off.
[ 5539.423904] CPU: 21 PID: 27658 Comm: tc Tainted: G        W         5.3.0-rc8+ #721
[ 5539.425400] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
[ 5539.426911] Call Trace:
[ 5539.428380]  dump_stack+0x85/0xc0
[ 5539.429823]  ___might_sleep.cold+0xac/0xbc
[ 5539.431262]  __mutex_lock+0x5b/0x960
[ 5539.432682]  ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 5539.434103]  ? __nla_validate_parse+0x51/0x840
[ 5539.435493]  ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 5539.436903]  tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 5539.438327]  tcf_block_put_ext.part.0+0x21/0x50
[ 5539.439752]  tcf_block_put+0x50/0x70
[ 5539.441165]  sfq_destroy+0x15/0x50 [sch_sfq]
[ 5539.442570]  qdisc_destroy+0x5f/0x160
[ 5539.444000]  multiq_tune+0x14a/0x420 [sch_multiq]
[ 5539.445421]  tc_modify_qdisc+0x324/0x840
[ 5539.446841]  rtnetlink_rcv_msg+0x170/0x4b0
[ 5539.448269]  ? netlink_deliver_tap+0x95/0x400
[ 5539.449691]  ? rtnl_dellink+0x2d0/0x2d0
[ 5539.451116]  netlink_rcv_skb+0x49/0x110
[ 5539.452522]  netlink_unicast+0x171/0x200
[ 5539.453914]  netlink_sendmsg+0x224/0x3f0
[ 5539.455304]  sock_sendmsg+0x5e/0x60
[ 5539.456686]  ___sys_sendmsg+0x2ae/0x330
[ 5539.458071]  ? ___sys_recvmsg+0x159/0x1f0
[ 5539.459461]  ? do_wp_page+0x9c/0x790
[ 5539.460846]  ? __handle_mm_fault+0xcd3/0x19e0
[ 5539.462263]  __sys_sendmsg+0x59/0xa0
[ 5539.463661]  do_syscall_64+0x5c/0xb0
[ 5539.465044]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 5539.466454] RIP: 0033:0x7f1fe08177b8
[ 5539.467863] Code: 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 65 8f 0c 00 8b 00 85 c0 75 17 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 48 83 ec 28 89 5
4
[ 5539.470906] RSP: 002b:00007ffe812de5d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 5539.472483] RAX: ffffffffffffffda RBX: 000000005d8135e3 RCX: 00007f1fe08177b8
[ 5539.474069] RDX: 0000000000000000 RSI: 00007ffe812de640 RDI: 0000000000000003
[ 5539.475655] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000182e9b0
[ 5539.477203] R10: 0000000000404eda R11: 0000000000000246 R12: 0000000000000001
[ 5539.478699] R13: 000000000047f640 R14: 0000000000000000 R15: 0000000000000000

Rearrange locking in multiq_tune() in following ways:

- Don't hold sch tree lock for whole duration of the loop that removes
  Qdiscs from queues that have been disabled. Obtain the lock each time
  parent qdisc is changed and release it afterwards.

- Move call to qdisc_put() after sch_tree_unlock(sch) in loop that adds
  Qdiscs on newly enabled tx queues. Since qdisc_put() implementation
  already verifies that argument Qdisc is not of builtin type before
  processing it we don't need additional conditional around the call here.

Fixes: c266f64dbfa2 ("net: sched: protect block state with mutex")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
 net/sched/sch_multiq.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_multiq.c b/net/sched/sch_multiq.c
index e1087746f6a2..4cfa9a7bd29e 100644
--- a/net/sched/sch_multiq.c
+++ b/net/sched/sch_multiq.c
@@ -187,18 +187,21 @@ static int multiq_tune(struct Qdisc *sch, struct nlattr *opt,
 
 	sch_tree_lock(sch);
 	q->bands = qopt->bands;
+	sch_tree_unlock(sch);
+
 	for (i = q->bands; i < q->max_bands; i++) {
 		if (q->queues[i] != &noop_qdisc) {
 			struct Qdisc *child = q->queues[i];
 
+			sch_tree_lock(sch);
 			q->queues[i] = &noop_qdisc;
 			qdisc_tree_flush_backlog(child);
+			sch_tree_unlock(sch);
+
 			qdisc_put(child);
 		}
 	}
 
-	sch_tree_unlock(sch);
-
 	for (i = 0; i < q->bands; i++) {
 		if (q->queues[i] == &noop_qdisc) {
 			struct Qdisc *child, *old;
@@ -213,11 +216,10 @@ static int multiq_tune(struct Qdisc *sch, struct nlattr *opt,
 				if (child != &noop_qdisc)
 					qdisc_hash_add(child, true);
 
-				if (old != &noop_qdisc) {
+				if (old != &noop_qdisc)
 					qdisc_tree_flush_backlog(old);
-					qdisc_put(old);
-				}
 				sch_tree_unlock(sch);
+				qdisc_put(old);
 			}
 		}
 	}
-- 
2.21.0


^ permalink raw reply related

* [PATCH net 3/3] net: sched: sch_sfb: don't call qdisc_put() while holding tree lock
From: Vlad Buslov @ 2019-09-18  7:32 UTC (permalink / raw)
  To: netdev
  Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov,
	syzbot+ac54455281db908c581e
In-Reply-To: <20190918073201.2320-1-vladbu@mellanox.com>

Recent changes that removed rtnl dependency from rules update path of tc
also made tcf_block_put() function sleeping. This function is called from
ops->destroy() of several Qdisc implementations, which in turn is called by
qdisc_put(). Some Qdiscs call qdisc_put() while holding sch tree spinlock,
which results sleeping-while-atomic BUG.

Steps to reproduce for sfb:

tc qdisc add dev ens1f0 handle 1: root sfb
tc qdisc add dev ens1f0 parent 1:10 handle 50: sfq perturb 10
tc qdisc change dev ens1f0 root handle 1: sfb

Resulting dmesg:

[ 7265.938717] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:909
[ 7265.940152] in_atomic(): 1, irqs_disabled(): 0, pid: 28579, name: tc
[ 7265.941455] INFO: lockdep is turned off.
[ 7265.942744] CPU: 11 PID: 28579 Comm: tc Tainted: G        W         5.3.0-rc8+ #721
[ 7265.944065] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
[ 7265.945396] Call Trace:
[ 7265.946709]  dump_stack+0x85/0xc0
[ 7265.947994]  ___might_sleep.cold+0xac/0xbc
[ 7265.949282]  __mutex_lock+0x5b/0x960
[ 7265.950543]  ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 7265.951803]  ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 7265.953022]  tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 7265.954248]  tcf_block_put_ext.part.0+0x21/0x50
[ 7265.955478]  tcf_block_put+0x50/0x70
[ 7265.956694]  sfq_destroy+0x15/0x50 [sch_sfq]
[ 7265.957898]  qdisc_destroy+0x5f/0x160
[ 7265.959099]  sfb_change+0x175/0x330 [sch_sfb]
[ 7265.960304]  tc_modify_qdisc+0x324/0x840
[ 7265.961503]  rtnetlink_rcv_msg+0x170/0x4b0
[ 7265.962692]  ? netlink_deliver_tap+0x95/0x400
[ 7265.963876]  ? rtnl_dellink+0x2d0/0x2d0
[ 7265.965064]  netlink_rcv_skb+0x49/0x110
[ 7265.966251]  netlink_unicast+0x171/0x200
[ 7265.967427]  netlink_sendmsg+0x224/0x3f0
[ 7265.968595]  sock_sendmsg+0x5e/0x60
[ 7265.969753]  ___sys_sendmsg+0x2ae/0x330
[ 7265.970916]  ? ___sys_recvmsg+0x159/0x1f0
[ 7265.972074]  ? do_wp_page+0x9c/0x790
[ 7265.973233]  ? __handle_mm_fault+0xcd3/0x19e0
[ 7265.974407]  __sys_sendmsg+0x59/0xa0
[ 7265.975591]  do_syscall_64+0x5c/0xb0
[ 7265.976753]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 7265.977938] RIP: 0033:0x7f229069f7b8
[ 7265.979117] Code: 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 65 8f 0c 00 8b 00 85 c0 75 17 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 48 83 ec 28 89 5
4
[ 7265.981681] RSP: 002b:00007ffd7ed2d158 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 7265.983001] RAX: ffffffffffffffda RBX: 000000005d813ca1 RCX: 00007f229069f7b8
[ 7265.984336] RDX: 0000000000000000 RSI: 00007ffd7ed2d1c0 RDI: 0000000000000003
[ 7265.985682] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000165c9a0
[ 7265.987021] R10: 0000000000404eda R11: 0000000000000246 R12: 0000000000000001
[ 7265.988309] R13: 000000000047f640 R14: 0000000000000000 R15: 0000000000000000

Save Qdisc that is being removed to temporary local variable and call
qdisc_put() with it after sch tree lock is released.

Reported-by: syzbot+ac54455281db908c581e@syzkaller.appspotmail.com
Fixes: c266f64dbfa2 ("net: sched: protect block state with mutex")
Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
 net/sched/sch_sfb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c
index 1dff8506a715..802a34afece0 100644
--- a/net/sched/sch_sfb.c
+++ b/net/sched/sch_sfb.c
@@ -488,7 +488,7 @@ static int sfb_change(struct Qdisc *sch, struct nlattr *opt,
 		      struct netlink_ext_ack *extack)
 {
 	struct sfb_sched_data *q = qdisc_priv(sch);
-	struct Qdisc *child;
+	struct Qdisc *child, *old;
 	struct nlattr *tb[TCA_SFB_MAX + 1];
 	const struct tc_sfb_qopt *ctl = &sfb_default_ops;
 	u32 limit;
@@ -519,7 +519,7 @@ static int sfb_change(struct Qdisc *sch, struct nlattr *opt,
 	sch_tree_lock(sch);
 
 	qdisc_tree_flush_backlog(q->qdisc);
-	qdisc_put(q->qdisc);
+	old = q->qdisc;
 	q->qdisc = child;
 
 	q->rehash_interval = msecs_to_jiffies(ctl->rehash_interval);
@@ -542,6 +542,7 @@ static int sfb_change(struct Qdisc *sch, struct nlattr *opt,
 	sfb_init_perturbation(1, q);
 
 	sch_tree_unlock(sch);
+	qdisc_put(old);
 
 	return 0;
 }
-- 
2.21.0


^ permalink raw reply related

* [PATCH net 0/3] Fix Qdisc destroy issues caused by adding fine-grained locking to filter API
From: Vlad Buslov @ 2019-09-18  7:31 UTC (permalink / raw)
  To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov

TC filter API unlocking introduced several new fine-grained locks. The
change caused sleeping-while-atomic BUGs in several Qdiscs that call cls
APIs which need to obtain new mutex while holding sch tree spinlock. This
series fixes affected Qdiscs by ensuring that cls API that became sleeping
is only called outside of sch tree lock critical section.

Vlad Buslov (3):
  net: sched: sch_htb: don't call qdisc_put() while holding tree lock
  net: sched: multiq: don't call qdisc_put() while holding tree lock
  net: sched: sch_sfb: don't call qdisc_put() while holding tree lock

 net/sched/sch_htb.c    |  4 +++-
 net/sched/sch_multiq.c | 12 +++++++-----
 net/sched/sch_sfb.c    |  5 +++--
 3 files changed, 13 insertions(+), 8 deletions(-)

-- 
2.21.0


^ permalink raw reply

* [PATCH RFC v3 3/5] net: Add a netdev software feature set that defaults to off.
From: Steffen Klassert @ 2019-09-18  7:25 UTC (permalink / raw)
  To: netdev; +Cc: Steffen Klassert, Willem de Bruijn, Paolo Abeni
In-Reply-To: <20190918072517.16037-1-steffen.klassert@secunet.com>

The previous patch added the NETIF_F_GRO_LIST feature.
This is a software feature that should default to off.
Current software features default to on, so add a new
feature set that defaults to off.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/linux/netdev_features.h | 3 +++
 net/core/dev.c                  | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 1b6baa1b6fe9..e8b3c943d835 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -228,6 +228,9 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
 /* changeable features with no special hardware requirements */
 #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
 
+/* Changeable features with no special hardware requirements that defaults to off. */
+#define NETIF_F_SOFT_FEATURES_OFF	NETIF_F_GRO_LIST
+
 #define NETIF_F_VLAN_FEATURES	(NETIF_F_HW_VLAN_CTAG_FILTER | \
 				 NETIF_F_HW_VLAN_CTAG_RX | \
 				 NETIF_F_HW_VLAN_CTAG_TX | \
diff --git a/net/core/dev.c b/net/core/dev.c
index b1afafee3e2a..cc0bbec0f1d7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8730,7 +8730,7 @@ int register_netdevice(struct net_device *dev)
 	/* Transfer changeable features to wanted_features and enable
 	 * software offloads (GSO and GRO).
 	 */
-	dev->hw_features |= NETIF_F_SOFT_FEATURES;
+	dev->hw_features |= (NETIF_F_SOFT_FEATURES | NETIF_F_SOFT_FEATURES_OFF);
 	dev->features |= NETIF_F_SOFT_FEATURES;
 
 	if (dev->netdev_ops->ndo_udp_tunnel_add) {
-- 
2.17.1


^ permalink raw reply related

* [PATCH RFC v3 5/5] udp: Support UDP fraglist GRO/GSO.
From: Steffen Klassert @ 2019-09-18  7:25 UTC (permalink / raw)
  To: netdev; +Cc: Steffen Klassert, Willem de Bruijn, Paolo Abeni
In-Reply-To: <20190918072517.16037-1-steffen.klassert@secunet.com>

This patch extends UDP GRO to support fraglist GRO/GSO
by using the previously introduced infrastructure.
All UDP packets that are not targeted to a GRO capable
UDP sockets are going to fraglist GRO now (local input
and forward).

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv4/udp_offload.c | 61 +++++++++++++++++++++++++++++++++++++-----
 net/ipv6/udp_offload.c |  9 +++++++
 2 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index e0b835890507..37daafb06d4c 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -184,6 +184,20 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 }
 EXPORT_SYMBOL(skb_udp_tunnel_segment);
 
+static struct sk_buff *__udp_gso_segment_list(struct sk_buff *skb,
+					      netdev_features_t features)
+{
+	unsigned int mss = skb_shinfo(skb)->gso_size;
+
+	skb = skb_segment_list(skb, features, skb_mac_header_len(skb));
+	if (IS_ERR(skb))
+		return skb;
+
+	udp_hdr(skb)->len = htons(sizeof(struct udphdr) + mss);
+
+	return skb;
+}
+
 struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 				  netdev_features_t features)
 {
@@ -196,6 +210,9 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 	__sum16 check;
 	__be16 newlen;
 
+	if (skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST)
+		return __udp_gso_segment_list(gso_skb, features);
+
 	mss = skb_shinfo(gso_skb)->gso_size;
 	if (gso_skb->len <= sizeof(*uh) + mss)
 		return ERR_PTR(-EINVAL);
@@ -354,6 +371,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
 	struct udphdr *uh2;
 	struct sk_buff *p;
 	unsigned int ulen;
+	int ret = 0;
 
 	/* requires non zero csum, for symmetry with GSO */
 	if (!uh->check) {
@@ -369,7 +387,6 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
 	}
 	/* pull encapsulating udp header */
 	skb_gro_pull(skb, sizeof(struct udphdr));
-	skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
 
 	list_for_each_entry(p, head, list) {
 		if (!NAPI_GRO_CB(p)->same_flow)
@@ -383,14 +400,35 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
 			continue;
 		}
 
+		if (NAPI_GRO_CB(skb)->is_flist != NAPI_GRO_CB(p)->is_flist) {
+			NAPI_GRO_CB(skb)->flush = 1;
+			return p;
+		}
+
 		/* Terminate the flow on len mismatch or if it grow "too much".
 		 * Under small packet flood GRO count could elsewhere grow a lot
 		 * leading to excessive truesize values.
 		 * On len mismatch merge the first packet shorter than gso_size,
 		 * otherwise complete the GRO packet.
 		 */
-		if (ulen > ntohs(uh2->len) || skb_gro_receive(p, skb) ||
-		    ulen != ntohs(uh2->len) ||
+		if (ulen > ntohs(uh2->len)) {
+			pp = p;
+		} else {
+			if (NAPI_GRO_CB(skb)->is_flist) {
+				if (!pskb_may_pull(skb, skb_gro_offset(skb))) {
+					NAPI_GRO_CB(skb)->flush = 1;
+					return NULL;
+				}
+				ret = skb_gro_receive_list(p, skb);
+			} else {
+				skb_gro_postpull_rcsum(skb, uh,
+						       sizeof(struct udphdr));
+
+				ret = skb_gro_receive(p, skb);
+			}
+		}
+
+		if (ret || ulen != ntohs(uh2->len) ||
 		    NAPI_GRO_CB(p)->count >= UDP_GRO_CNT_MAX)
 			pp = p;
 
@@ -411,6 +449,9 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
 	int flush = 1;
 
 	if (!sk || !udp_sk(sk)->gro_receive) {
+		if (skb->dev->features & NETIF_F_GRO_LIST)
+			NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
+
 		pp = call_gro_receive(udp_gro_receive_segment, head, skb);
 		return pp;
 	}
@@ -419,7 +460,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
 	    (skb->ip_summed != CHECKSUM_PARTIAL &&
 	     NAPI_GRO_CB(skb)->csum_cnt == 0 &&
 	     !NAPI_GRO_CB(skb)->csum_valid))
-		goto out_unlock;
+		goto out;
 
 	/* mark that this skb passed once through the tunnel gro layer */
 	NAPI_GRO_CB(skb)->encap_mark = 1;
@@ -446,8 +487,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
 	skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
 	pp = call_gro_receive_sk(udp_sk(sk)->gro_receive, sk, head, skb);
 
-out_unlock:
-	rcu_read_unlock();
+out:
 	skb_gro_flush_final(skb, pp, flush);
 	return pp;
 }
@@ -539,6 +579,15 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
 	const struct iphdr *iph = ip_hdr(skb);
 	struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
 
+	if (NAPI_GRO_CB(skb)->is_flist) {
+		uh->len = htons(skb->len - nhoff);
+
+		skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);
+		skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count;
+
+		return 0;
+	}
+
 	if (uh->check)
 		uh->check = ~udp_v4_check(skb->len - nhoff, iph->saddr,
 					  iph->daddr, 0);
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index 47a3b1348371..e503b5cf9023 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -150,6 +150,15 @@ INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff)
 	const struct ipv6hdr *ipv6h = ipv6_hdr(skb);
 	struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
 
+	if (NAPI_GRO_CB(skb)->is_flist) {
+		uh->len = htons(skb->len - nhoff);
+
+		skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);
+		skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count;
+
+		return 0;
+	}
+
 	if (uh->check)
 		uh->check = ~udp_v6_check(skb->len - nhoff, &ipv6h->saddr,
 					  &ipv6h->daddr, 0);
-- 
2.17.1


^ permalink raw reply related

* [PATCH RFC v3 4/5] net: Support GRO/GSO fraglist chaining.
From: Steffen Klassert @ 2019-09-18  7:25 UTC (permalink / raw)
  To: netdev; +Cc: Steffen Klassert, Willem de Bruijn, Paolo Abeni
In-Reply-To: <20190918072517.16037-1-steffen.klassert@secunet.com>

This patch adds the core functions to chain/unchain
GSO skbs at the frag_list pointer. This also adds
a new GSO type SKB_GSO_FRAGLIST and a is_flist
flag to napi_gro_cb which indicates that this
flow will be GROed by fraglist chaining.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/linux/netdevice.h |   4 +-
 include/linux/skbuff.h    |   4 ++
 net/core/dev.c            |   2 +-
 net/core/skbuff.c         | 106 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 114 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d7d5626002e9..e97ca3e51466 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2306,7 +2306,8 @@ struct napi_gro_cb {
 	/* Number of gro_receive callbacks this packet already went through */
 	u8 recursion_counter:4;
 
-	/* 1 bit hole */
+	/* GRO is done by frag_list pointer chaining. */
+	u8	is_flist:1;
 
 	/* used to support CHECKSUM_COMPLETE for tunneling protocols */
 	__wsum	csum;
@@ -2656,6 +2657,7 @@ struct net_device *dev_get_by_napi_id(unsigned int napi_id);
 int netdev_get_name(struct net *net, char *name, int ifindex);
 int dev_restart(struct net_device *dev);
 int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb);
+int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb);
 
 static inline unsigned int skb_gro_offset(const struct sk_buff *skb)
 {
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 028e684fa974..3d5fd0a0eea7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -595,6 +595,8 @@ enum {
 	SKB_GSO_UDP = 1 << 16,
 
 	SKB_GSO_UDP_L4 = 1 << 17,
+
+	SKB_GSO_FRAGLIST = 1 << 18,
 };
 
 #if BITS_PER_LONG > 32
@@ -3509,6 +3511,8 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet);
 bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu);
 bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
 struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
+struct sk_buff *skb_segment_list(struct sk_buff *skb, netdev_features_t features,
+				 unsigned int offset);
 struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
 int skb_ensure_writable(struct sk_buff *skb, int write_len);
 int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci);
diff --git a/net/core/dev.c b/net/core/dev.c
index cc0bbec0f1d7..f2a66198154d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3109,7 +3109,7 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
 
 	segs = skb_mac_gso_segment(skb, features);
 
-	if (unlikely(skb_needs_check(skb, tx_path) && !IS_ERR(segs)))
+	if (segs != skb && unlikely(skb_needs_check(skb, tx_path) && !IS_ERR(segs)))
 		skb_warn_bad_offload(skb);
 
 	return segs;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 2b40b5a9425b..3ff56677a6fb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3638,6 +3638,112 @@ static inline skb_frag_t skb_head_frag_to_page_desc(struct sk_buff *frag_skb)
 	return head_frag;
 }
 
+struct sk_buff *skb_segment_list(struct sk_buff *skb,
+				 netdev_features_t features,
+				 unsigned int offset)
+{
+	struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
+	unsigned int tnl_hlen = skb_tnl_header_len(skb);
+	unsigned int delta_truesize = 0;
+	unsigned int delta_len = 0;
+	struct sk_buff *tail = NULL;
+	struct sk_buff *nskb;
+
+	skb_push(skb, -skb_network_offset(skb) + offset);
+
+	skb_shinfo(skb)->frag_list = NULL;
+
+	do {
+		nskb = list_skb;
+		list_skb = list_skb->next;
+
+		if (!tail)
+			skb->next = nskb;
+		else
+			tail->next = nskb;
+
+		tail = nskb;
+
+		delta_len += nskb->len;
+		delta_truesize += nskb->truesize;
+
+		skb_push(nskb, -skb_network_offset(nskb) + offset);
+
+		if (!secpath_exists(nskb))
+			__skb_ext_copy(nskb, skb);
+
+		memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
+
+		nskb->ip_summed = skb->ip_summed;
+		nskb->csum_valid = skb->csum_valid;
+		nskb->tstamp = skb->tstamp;
+		nskb->dev = skb->dev;
+		nskb->queue_mapping = skb->queue_mapping;
+
+		nskb->mac_len = skb->mac_len;
+		nskb->mac_header = skb->mac_header;
+		nskb->transport_header = skb->transport_header;
+		nskb->network_header = skb->network_header;
+		skb_dst_copy(nskb, skb);
+
+		skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
+		skb_copy_from_linear_data_offset(skb, -tnl_hlen,
+						 nskb->data - tnl_hlen,
+						 offset + tnl_hlen);
+
+		if (skb_needs_linearize(nskb, features) &&
+		    __skb_linearize(nskb))
+			goto err_linearize;
+
+	} while (list_skb);
+
+	skb->truesize = skb->truesize - delta_truesize;
+	skb->data_len = skb->data_len - delta_len;
+	skb->len = skb->len - delta_len;
+
+	skb_gso_reset(skb);
+
+	skb->prev = tail;
+
+	if (skb_needs_linearize(skb, features) &&
+	    __skb_linearize(skb))
+		goto err_linearize;
+
+	skb_get(skb);
+
+	return skb;
+
+err_linearize:
+	kfree_skb_list(skb->next);
+	skb->next = NULL;
+	return ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL_GPL(skb_segment_list);
+
+int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
+{
+	if (unlikely(p->len + skb->len >= 65536))
+		return -E2BIG;
+
+	if (NAPI_GRO_CB(p)->last == p)
+		skb_shinfo(p)->frag_list = skb;
+	else
+		NAPI_GRO_CB(p)->last->next = skb;
+
+	skb_pull(skb, skb_gro_offset(skb));
+
+	NAPI_GRO_CB(p)->last = skb;
+	NAPI_GRO_CB(p)->count++;
+	p->data_len += skb->len;
+	p->truesize += skb->truesize;
+	p->len += skb->len;
+
+	NAPI_GRO_CB(skb)->same_flow = 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(skb_gro_receive_list);
+
 /**
  *	skb_segment - Perform protocol segmentation on skb.
  *	@head_skb: buffer to segment
-- 
2.17.1


^ permalink raw reply related

* [PATCH RFC v3 1/5] UDP: enable GRO by default.
From: Steffen Klassert @ 2019-09-18  7:25 UTC (permalink / raw)
  To: netdev; +Cc: Steffen Klassert, Willem de Bruijn, Paolo Abeni
In-Reply-To: <20190918072517.16037-1-steffen.klassert@secunet.com>

This patch enables UDP GRO regardless if a GRO capable
socket is present. With this GRO is done by default
for the local input and forwarding path.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/net/udp.h      |  2 +-
 net/ipv4/udp_offload.c | 33 ++++++++++++++-------------------
 net/ipv6/udp_offload.c | 10 ++++++++--
 3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/include/net/udp.h b/include/net/udp.h
index bad74f780831..44e0e52b585c 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -167,7 +167,7 @@ typedef struct sock *(*udp_lookup_t)(struct sk_buff *skb, __be16 sport,
 				     __be16 dport);
 
 struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
-				struct udphdr *uh, udp_lookup_t lookup);
+				struct udphdr *uh, struct sock *sk);
 int udp_gro_complete(struct sk_buff *skb, int nhoff, udp_lookup_t lookup);
 
 struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index a3908e55ed89..e0b835890507 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -401,35 +401,24 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
 	return NULL;
 }
 
-INDIRECT_CALLABLE_DECLARE(struct sock *udp6_lib_lookup_skb(struct sk_buff *skb,
-						   __be16 sport, __be16 dport));
 struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
-				struct udphdr *uh, udp_lookup_t lookup)
+				struct udphdr *uh, struct sock *sk)
 {
 	struct sk_buff *pp = NULL;
 	struct sk_buff *p;
 	struct udphdr *uh2;
 	unsigned int off = skb_gro_offset(skb);
 	int flush = 1;
-	struct sock *sk;
-
-	rcu_read_lock();
-	sk = INDIRECT_CALL_INET(lookup, udp6_lib_lookup_skb,
-				udp4_lib_lookup_skb, skb, uh->source, uh->dest);
-	if (!sk)
-		goto out_unlock;
 
-	if (udp_sk(sk)->gro_enabled) {
+	if (!sk || !udp_sk(sk)->gro_receive) {
 		pp = call_gro_receive(udp_gro_receive_segment, head, skb);
-		rcu_read_unlock();
 		return pp;
 	}
 
 	if (NAPI_GRO_CB(skb)->encap_mark ||
 	    (skb->ip_summed != CHECKSUM_PARTIAL &&
 	     NAPI_GRO_CB(skb)->csum_cnt == 0 &&
-	     !NAPI_GRO_CB(skb)->csum_valid) ||
-	    !udp_sk(sk)->gro_receive)
+	     !NAPI_GRO_CB(skb)->csum_valid))
 		goto out_unlock;
 
 	/* mark that this skb passed once through the tunnel gro layer */
@@ -468,8 +457,10 @@ INDIRECT_CALLABLE_SCOPE
 struct sk_buff *udp4_gro_receive(struct list_head *head, struct sk_buff *skb)
 {
 	struct udphdr *uh = udp_gro_udphdr(skb);
+	struct sk_buff *pp;
+	struct sock *sk;
 
-	if (unlikely(!uh) || !static_branch_unlikely(&udp_encap_needed_key))
+	if (unlikely(!uh))
 		goto flush;
 
 	/* Don't bother verifying checksum if we're going to flush anyway. */
@@ -484,7 +475,11 @@ struct sk_buff *udp4_gro_receive(struct list_head *head, struct sk_buff *skb)
 					     inet_gro_compute_pseudo);
 skip:
 	NAPI_GRO_CB(skb)->is_ipv6 = 0;
-	return udp_gro_receive(head, skb, uh, udp4_lib_lookup_skb);
+	rcu_read_lock();
+	sk = static_branch_unlikely(&udp_encap_needed_key) ? udp4_lib_lookup_skb(skb, uh->source, uh->dest) : NULL;
+	pp = udp_gro_receive(head, skb, uh, sk);
+	rcu_read_unlock();
+	return pp;
 
 flush:
 	NAPI_GRO_CB(skb)->flush = 1;
@@ -517,9 +512,7 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff,
 	rcu_read_lock();
 	sk = INDIRECT_CALL_INET(lookup, udp6_lib_lookup_skb,
 				udp4_lib_lookup_skb, skb, uh->source, uh->dest);
-	if (sk && udp_sk(sk)->gro_enabled) {
-		err = udp_gro_complete_segment(skb);
-	} else if (sk && udp_sk(sk)->gro_complete) {
+	if (sk && udp_sk(sk)->gro_complete) {
 		skb_shinfo(skb)->gso_type = uh->check ? SKB_GSO_UDP_TUNNEL_CSUM
 					: SKB_GSO_UDP_TUNNEL;
 
@@ -529,6 +522,8 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff,
 		skb->encapsulation = 1;
 		err = udp_sk(sk)->gro_complete(sk, skb,
 				nhoff + sizeof(struct udphdr));
+	} else {
+		err = udp_gro_complete_segment(skb);
 	}
 	rcu_read_unlock();
 
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index 64b8f05d6735..47a3b1348371 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -115,8 +115,10 @@ INDIRECT_CALLABLE_SCOPE
 struct sk_buff *udp6_gro_receive(struct list_head *head, struct sk_buff *skb)
 {
 	struct udphdr *uh = udp_gro_udphdr(skb);
+	struct sk_buff *pp;
+	struct sock *sk;
 
-	if (unlikely(!uh) || !static_branch_unlikely(&udpv6_encap_needed_key))
+	if (unlikely(!uh))
 		goto flush;
 
 	/* Don't bother verifying checksum if we're going to flush anyway. */
@@ -132,7 +134,11 @@ struct sk_buff *udp6_gro_receive(struct list_head *head, struct sk_buff *skb)
 
 skip:
 	NAPI_GRO_CB(skb)->is_ipv6 = 1;
-	return udp_gro_receive(head, skb, uh, udp6_lib_lookup_skb);
+	rcu_read_lock();
+	sk = static_branch_unlikely(&udp_encap_needed_key) ? udp6_lib_lookup_skb(skb, uh->source, uh->dest) : NULL;
+	pp = udp_gro_receive(head, skb, uh, sk);
+	rcu_read_unlock();
+	return pp;
 
 flush:
 	NAPI_GRO_CB(skb)->flush = 1;
-- 
2.17.1


^ permalink raw reply related

* [PATCH RFC v3 0/5] Support fraglist GRO/GSO
From: Steffen Klassert @ 2019-09-18  7:25 UTC (permalink / raw)
  To: netdev; +Cc: Steffen Klassert, Willem de Bruijn, Paolo Abeni

This patchset adds support to do GRO/GSO by chaining packets
of the same flow at the SKB frag_list pointer. This avoids
the overhead to merge payloads into one big packet, and
on the other end, if GSO is needed it avoids the overhead
of splitting the big packet back to the native form.

Patch 1 Enables UDP GRO by default.

Patch 2 adds a netdev feature flag to enable listifyed GRO,
this implements one of the configuration options discussed
at netconf 2019.

Patch 3 adds a netdev software feature set that defaults to off
and assigns the new listifyed GRO feature flag to it.

Patch 4 adds the core infrastructure to do fraglist GRO/GSO.

Patch 5 enables UDP to use fraglist GRO/GSO if configured and no
GRO supported socket is found.

I have only meaningful forwarding performance measurements.
I did some tests for the local receive path with netperf and iperf,
but in this case the sender that generates the packets is the
bottleneck. So the benchmarks are not that meaningful for the
receive path.

Paolo Abeni did some benchmarks of the local receive path for the v2
version of this pachset, results can be found here:

https://www.spinics.net/lists/netdev/msg551158.html

I used my IPsec forwarding test setup for the performance measurements:

           ------------         ------------
        -->| router 1 |-------->| router 2 |--
        |  ------------         ------------  |
        |                                     |
        |       --------------------          |
        --------|Spirent Testcenter|<----------
                --------------------

net-next (September 7th):

Single stream UDP frame size 1460 Bytes: 1.161.000 fps (13.5 Gbps).

----------------------------------------------------------------------

net-next (September 7th) + standard UDP GRO/GSO:

Single stream UDP frame size 1460 Bytes: 1.801.000 fps (21 Gbps).

----------------------------------------------------------------------

net-next (September 7th) + fraglist UDP GRO/GSO:

Single stream UDP frame size 1460 Bytes: 2.860.000 fps (33.4 Gbps).

-----------------------------------------------------------------------

Changes from v1:

- Add IPv6 support.
- Split patchset to enable UDP GRO by default before adding
  fraglist GRO support.
- Mark fraglist GRO packets as CHECKSUM_NONE.
- Take a refcount on the first segment skb when doing fraglist
  segmentation. With this we can use the same error handling
  path as with standard segmentation.

Changes from v2:

- Add a netdev feature flag to configure listifyed GRO.
- Fix UDP GRO enabling for IPv6.
- Fix a rcu_read_lock() imbalance.
- Fix error path in skb_segment_list().

^ permalink raw reply

* [PATCH RFC v3 2/5] net: Add NETIF_F_GRO_LIST feature
From: Steffen Klassert @ 2019-09-18  7:25 UTC (permalink / raw)
  To: netdev; +Cc: Steffen Klassert, Willem de Bruijn, Paolo Abeni
In-Reply-To: <20190918072517.16037-1-steffen.klassert@secunet.com>

This adds a new NETIF_F_GRO_LIST feature flag. I will be used
to configure listfyed GRO what will be implemented with some
followup paches.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/linux/netdev_features.h | 2 ++
 net/core/ethtool.c              | 1 +
 2 files changed, 3 insertions(+)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 4b19c544c59a..1b6baa1b6fe9 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -80,6 +80,7 @@ enum {
 
 	NETIF_F_GRO_HW_BIT,		/* Hardware Generic receive offload */
 	NETIF_F_HW_TLS_RECORD_BIT,	/* Offload TLS record */
+	NETIF_F_GRO_LIST_BIT,		/* Listifyed GRO */
 
 	/*
 	 * Add your fresh new feature above and remember to update
@@ -150,6 +151,7 @@ enum {
 #define NETIF_F_GSO_UDP_L4	__NETIF_F(GSO_UDP_L4)
 #define NETIF_F_HW_TLS_TX	__NETIF_F(HW_TLS_TX)
 #define NETIF_F_HW_TLS_RX	__NETIF_F(HW_TLS_RX)
+#define NETIF_F_GRO_LIST	__NETIF_F(GRO_LIST)
 
 /* Finds the next feature with the highest number of the range of start till 0.
  */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 6288e69e94fc..ee8d2b58c2d7 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -111,6 +111,7 @@ static const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN]
 	[NETIF_F_HW_TLS_RECORD_BIT] =	"tls-hw-record",
 	[NETIF_F_HW_TLS_TX_BIT] =	 "tls-hw-tx-offload",
 	[NETIF_F_HW_TLS_RX_BIT] =	 "tls-hw-rx-offload",
+	[NETIF_F_GRO_LIST_BIT] =         "rx-gro-list",
 };
 
 static const char
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH iproute2 4/4] devlink: Fix devlink health set command
From: Aya Levin @ 2019-09-18  7:07 UTC (permalink / raw)
  To: Stephen Hemminger, Tariq Toukan
  Cc: David Ahern, netdev@vger.kernel.org, Moshe Shemesh, Jiri Pirko
In-Reply-To: <20190917172357.5c70c3b9@xps13.home>



On 9/17/2019 6:23 PM, Stephen Hemminger wrote:
> On Thu,  5 Sep 2019 15:43:07 +0300
> Tariq Toukan <tariqt@mellanox.com> wrote:
> 
>> From: Aya Levin <ayal@mellanox.com>
>>
>> Prior to this patch both the reporter's name and the grace period
>> attributes shared the same bit. This caused zeroing grace period when
>> setting auto recovery. Let each parameter has its own bit.
>>
>> Fixes: b18d89195b16 ("devlink: Add devlink health set command")
>> Signed-off-by: Aya Levin <ayal@mellanox.com>
>> Acked-by: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> 
> Does not apply to current iproute2.
This patch is redundant due to patch from Andrea Claudi 
4fb98f08956ff4810354d75afa9b04cb6f8011bc
Please reject it.
Other patches that were submitted with, are valid.
> 

^ permalink raw reply

* Re: [PATCH v3 2/2] net/ibmvnic: prevent more than one thread from running in reset
From: Michael Ellerman @ 2019-09-18  6:12 UTC (permalink / raw)
  To: Juliet Kim, netdev; +Cc: julietk, linuxppc-dev, tlfalcon
In-Reply-To: <20190917171552.32498-3-julietk@linux.vnet.ibm.com>

Hi Juliet,

Juliet Kim <julietk@linux.vnet.ibm.com> writes:
> Signed-off-by: Juliet Kim <julietk@linux.vnet.ibm.com>
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 23 ++++++++++++++++++++++-
>  drivers/net/ethernet/ibm/ibmvnic.h |  3 +++
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index ba340aaff1b3..f344ccd68ad9 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -2054,6 +2054,13 @@ static void __ibmvnic_reset(struct work_struct *work)
>  
>  	adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset);
>  
> +	if (adapter->resetting) {
> +		schedule_delayed_work(&adapter->ibmvnic_delayed_reset,
> +				      IBMVNIC_RESET_DELAY);
> +		return;
> +	}
> +
> +	adapter->resetting = true;
>  	reset_state = adapter->state;

Is there some locking/serialisation around this?

Otherwise that looks very racy. ie. two CPUs could both see
adapter->resetting == false, then both set it to true, and then continue
executing and stomp on each other.

cheers

^ permalink raw reply

* RE: [PATCH 3/6] Timer: expose monotonic clock and counter value
From: Jianyong Wu (Arm Technology China) @ 2019-09-18  6:01 UTC (permalink / raw)
  To: Richard Cochran
  Cc: netdev@vger.kernel.org, pbonzini@redhat.com,
	sean.j.christopherson@intel.com, maz@kernel.org, Mark Rutland,
	Will Deacon, Suzuki Poulose, linux-kernel@vger.kernel.org,
	Steve Capper, Kaly Xin (Arm Technology China),
	Justin He (Arm Technology China), nd,
	linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190918034235.GA1469@localhost>

Hi Richard,

> -----Original Message-----
> From: Richard Cochran <richardcochran@gmail.com>
> Sent: Wednesday, September 18, 2019 11:43 AM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; maz@kernel.org; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>; linux-kernel@vger.kernel.org; Steve
> Capper <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 3/6] Timer: expose monotonic clock and counter value
> 
> On Tue, Sep 17, 2019 at 07:24:27AM -0400, Jianyong Wu wrote:
> > A number of PTP drivers (such as ptp-kvm) are assuming what the
> > current clock source is, which could lead to interesting effects on
> > systems where the clocksource can change depending on external events.
> >
> > For this purpose, add a new API that retrives both the current
> > monotonic clock as well as its counter value.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> >  include/linux/timekeeping.h |  3 +++
> >  kernel/time/timekeeping.c   | 13 +++++++++++++
> >  2 files changed, 16 insertions(+)
> 
> For core time keeping changes, you must CC lkml, tglx, and John Stultz.
> 

Thanks, I will.

Thanks
Jianyong Wu

> Thanks,
> Richard

^ permalink raw reply

* Re: [RFC PATCH 2/4] mdev: introduce helper to set per device dma ops
From: Jason Wang @ 2019-09-18  5:58 UTC (permalink / raw)
  To: Alex Williamson
  Cc: mst, kvm, virtualization, netdev, linux-kernel, kwankhede, cohuck,
	tiwei.bie, maxime.coquelin, cunming.liang, zhihong.wang,
	rob.miller, idos, xiao.w.wang, haotian.wang
In-Reply-To: <20190917130044.4fb97637@x1.home>


On 2019/9/18 上午3:00, Alex Williamson wrote:
> On Tue, 10 Sep 2019 16:19:33 +0800
> Jason Wang<jasowang@redhat.com>  wrote:
>
>> This patch introduces mdev_set_dma_ops() which allows parent to set
>> per device DMA ops. This help for the kernel driver to setup a correct
>> DMA mappings.
>>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> ---
>>   drivers/vfio/mdev/mdev_core.c | 7 +++++++
>>   include/linux/mdev.h          | 2 ++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>> index b558d4cfd082..eb28552082d7 100644
>> --- a/drivers/vfio/mdev/mdev_core.c
>> +++ b/drivers/vfio/mdev/mdev_core.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/uuid.h>
>>   #include <linux/sysfs.h>
>>   #include <linux/mdev.h>
>> +#include <linux/dma-mapping.h>
>>   
>>   #include "mdev_private.h"
>>   
>> @@ -27,6 +28,12 @@ static struct class_compat *mdev_bus_compat_class;
>>   static LIST_HEAD(mdev_list);
>>   static DEFINE_MUTEX(mdev_list_lock);
>>   
>> +void mdev_set_dma_ops(struct mdev_device *mdev, struct dma_map_ops *ops)
>> +{
>> +	set_dma_ops(&mdev->dev, ops);
>> +}
>> +EXPORT_SYMBOL(mdev_set_dma_ops);
>> +
> Why does mdev need to be involved here?  Your sample driver in 4/4 calls
> this from its create callback, where it could just as easily call:
>
>    set_dma_ops(mdev_dev(mdev), ops);
>
> Thanks,
> Alex


Yes, let me withdraw this patch.

Thanks


^ permalink raw reply

* Re: [PATCH] ionic: Remove unnecessary ternary operator in ionic_debugfs_add_ident
From: Greg Kroah-Hartman @ 2019-09-18  5:57 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Shannon Nelson, Pensando Drivers, David S. Miller, netdev,
	linux-kernel, clang-built-linux
In-Reply-To: <20190917232616.125261-1-natechancellor@gmail.com>

On Tue, Sep 17, 2019 at 04:26:16PM -0700, Nathan Chancellor wrote:
> clang warns:
> 
> ../drivers/net/ethernet/pensando/ionic/ionic_debugfs.c:60:37: warning:
> expression result unused [-Wunused-value]
>                             ionic, &identity_fops) ? 0 : -EOPNOTSUPP;
>                                                          ^~~~~~~~~~~
> 1 warning generated.
> 
> The return value of debugfs_create_file does not need to be checked [1]
> and the function returns void so get rid of the ternary operator, it is
> unnecessary.
> 
> [1]: https://lore.kernel.org/linux-mm/20150815160730.GB25186@kroah.com/
> 
> Fixes: fbfb8031533c ("ionic: Add hardware init and device commands")
> Link: https://github.com/ClangBuiltLinux/linux/issues/658
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply

* Re: Bug report (with fix) for DEC Tulip driver (de2104x.c)
From: Helge Deller @ 2019-09-18  5:56 UTC (permalink / raw)
  To: John David Anglin, Arlie Davis, Andrew Lunn; +Cc: netdev, linux-parisc
In-Reply-To: <df0f961d-2d53-63e3-8087-6f0b09e14317@bell.net>

On 18.09.19 00:51, John David Anglin wrote:
> On 2019-09-17 5:36 p.m., Arlie Davis wrote:
>> Likewise, I'm at a loss for testing with real hardware. It's hard to
>> find such things, now.
> How does de2104x compare to ds2142/43?  I have a c3750 with ds2142/43 tulip.  Helge
> or some others might have a machine with a de2104x.

The machines we could test are
* a C240 with a DS21140 tulip chip (Sven has one),
* a C3000 or similiar with DS21142 and/or DS21143 (me).

If the patch does not show any regressions, I'd suggest to
apply it upstream.

Helge

^ permalink raw reply

* Re: [RFC v4 0/3] vhost: introduce mdev based hardware backend
From: Jason Wang @ 2019-09-18  5:51 UTC (permalink / raw)
  To: Tiwei Bie
  Cc: mst, alex.williamson, maxime.coquelin, linux-kernel, kvm,
	virtualization, netdev, dan.daly, cunming.liang, zhihong.wang,
	lingshan.zhu
In-Reply-To: <20190917105801.GA24855@___>


On 2019/9/17 下午6:58, Tiwei Bie wrote:
> On Tue, Sep 17, 2019 at 11:32:03AM +0800, Jason Wang wrote:
>> On 2019/9/17 上午9:02, Tiwei Bie wrote:
>>> This RFC is to demonstrate below ideas,
>>>
>>> a) Build vhost-mdev on top of the same abstraction defined in
>>>      the virtio-mdev series [1];
>>>
>>> b) Introduce /dev/vhost-mdev to do vhost ioctls and support
>>>      setting mdev device as backend;
>>>
>>> Now the userspace API looks like this:
>>>
>>> - Userspace generates a compatible mdev device;
>>>
>>> - Userspace opens this mdev device with VFIO API (including
>>>     doing IOMMU programming for this mdev device with VFIO's
>>>     container/group based interface);
>>>
>>> - Userspace opens /dev/vhost-mdev and gets vhost fd;
>>>
>>> - Userspace uses vhost ioctls to setup vhost (userspace should
>>>     do VHOST_MDEV_SET_BACKEND ioctl with VFIO group fd and device
>>>     fd first before doing other vhost ioctls);
>>>
>>> Only compile test has been done for this series for now.
>>
>> Have a hard thought on the architecture:
> Thanks a lot! Do appreciate it!
>
>> 1) Create a vhost char device and pass vfio mdev device fd to it as a
>> backend and translate vhost-mdev ioctl to virtio mdev transport (e.g
>> read/write). DMA was done through the VFIO DMA mapping on the container that
>> is attached.
> Yeah, that's what we are doing in this series.
>
>> We have two more choices:
>>
>> 2) Use vfio-mdev but do not create vhost-mdev device, instead, just
>> implement vhost ioctl on vfio_device_ops, and translate them into
>> virtio-mdev transport or just pass ioctl to parent.
> Yeah. Instead of introducing /dev/vhost-mdev char device, do
> vhost ioctls on VFIO device fd directly. That's what we did
> in RFC v3.
>
>> 3) Don't use vfio-mdev, create a new vhost-mdev driver, during probe still
>> try to add dev to vfio group and talk to parent with device specific ops
> If my understanding is correct, this means we need to introduce
> a new VFIO device driver to replace the existing vfio-mdev driver
> in our case. Below is a quick draft just to show my understanding:
>
> #include <linux/init.h>
> #include <linux/module.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/vfio.h>
> #include <linux/mdev.h>
>
> #include "mdev_private.h"
>
> /* XXX: we need a proper way to include below vhost header. */
> #include "../../vhost/vhost.h"
>
> static int vfio_vhost_mdev_open(void *device_data)
> {
> 	if (!try_module_get(THIS_MODULE))
> 		return -ENODEV;
>
> 	/* ... */
> 	vhost_dev_init(...);
>
> 	return 0;
> }
>
> static void vfio_vhost_mdev_release(void *device_data)
> {
> 	/* ... */
> 	module_put(THIS_MODULE);
> }
>
> static long vfio_vhost_mdev_unlocked_ioctl(void *device_data,
> 					   unsigned int cmd, unsigned long arg)
> {
> 	struct mdev_device *mdev = device_data;
> 	struct mdev_parent *parent = mdev->parent;
>
> 	/*
> 	 * Use vhost ioctls.
> 	 *
> 	 * We will have a different parent_ops design.
> 	 * And potentially, we can share the same parent_ops
> 	 * with virtio_mdev.
> 	 */
> 	switch (cmd) {
> 	case VHOST_GET_FEATURES:
> 		parent->ops->get_features(mdev, ...);
> 		break;
> 	/* ... */
> 	}
>
> 	return 0;
> }
>
> static ssize_t vfio_vhost_mdev_read(void *device_data, char __user *buf,
> 				    size_t count, loff_t *ppos)
> {
> 	/* ... */
> 	return 0;
> }
>
> static ssize_t vfio_vhost_mdev_write(void *device_data, const char __user *buf,
> 				     size_t count, loff_t *ppos)
> {
> 	/* ... */
> 	return 0;
> }
>
> static int vfio_vhost_mdev_mmap(void *device_data, struct vm_area_struct *vma)
> {
> 	/* ... */
> 	return 0;
> }
>
> static const struct vfio_device_ops vfio_vhost_mdev_dev_ops = {
> 	.name		= "vfio-vhost-mdev",
> 	.open		= vfio_vhost_mdev_open,
> 	.release	= vfio_vhost_mdev_release,
> 	.ioctl		= vfio_vhost_mdev_unlocked_ioctl,
> 	.read		= vfio_vhost_mdev_read,
> 	.write		= vfio_vhost_mdev_write,
> 	.mmap		= vfio_vhost_mdev_mmap,
> };
>
> static int vfio_vhost_mdev_probe(struct device *dev)
> {
> 	struct mdev_device *mdev = to_mdev_device(dev);
>
> 	/* ... */
> 	return vfio_add_group_dev(dev, &vfio_vhost_mdev_dev_ops, mdev);
> }
>
> static void vfio_vhost_mdev_remove(struct device *dev)
> {
> 	/* ... */
> 	vfio_del_group_dev(dev);
> }
>
> static struct mdev_driver vfio_vhost_mdev_driver = {
> 	.name	= "vfio_vhost_mdev",
> 	.probe	= vfio_vhost_mdev_probe,
> 	.remove	= vfio_vhost_mdev_remove,
> };
>
> static int __init vfio_vhost_mdev_init(void)
> {
> 	return mdev_register_driver(&vfio_vhost_mdev_driver, THIS_MODULE);
> }
> module_init(vfio_vhost_mdev_init)
>
> static void __exit vfio_vhost_mdev_exit(void)
> {
> 	mdev_unregister_driver(&vfio_vhost_mdev_driver);
> }
> module_exit(vfio_vhost_mdev_exit)


Yes, something like this basically.


>> So I have some questions:
>>
>> 1) Compared to method 2, what's the advantage of creating a new vhost char
>> device? I guess it's for keep the API compatibility?
> One benefit is that we can avoid doing vhost ioctls on
> VFIO device fd.


Yes, but any benefit from doing this?


>
>> 2) For method 2, is there any easy way for user/admin to distinguish e.g
>> ordinary vfio-mdev for vhost from ordinary vfio-mdev?
> I think device-api could be a choice.


Ok.


>
>> I saw you introduce
>> ops matching helper but it's not friendly to management.
> The ops matching helper is just to check whether a given
> vfio-device is based on a mdev device.
>
>> 3) A drawback of 1) and 2) is that it must follow vfio_device_ops that
>> assumes the parameter comes from userspace, it prevents support kernel
>> virtio drivers.
>>
>> 4) So comes the idea of method 3, since it register a new vhost-mdev driver,
>> we can use device specific ops instead of VFIO ones, then we can have a
>> common API between vDPA parent and vhost-mdev/virtio-mdev drivers.
> As the above draft shows, this requires introducing a new
> VFIO device driver. I think Alex's opinion matters here.


Yes, it is.

Thanks


> Thanks,
> Tiwei
>
>> What's your thoughts?
>>
>> Thanks
>>
>>
>>> RFCv3: https://patchwork.kernel.org/patch/11117785/
>>>
>>> [1] https://lkml.org/lkml/2019/9/10/135
>>>
>>> Tiwei Bie (3):
>>>     vfio: support getting vfio device from device fd
>>>     vfio: support checking vfio driver by device ops
>>>     vhost: introduce mdev based hardware backend
>>>
>>>    drivers/vfio/mdev/vfio_mdev.c    |   3 +-
>>>    drivers/vfio/vfio.c              |  32 +++
>>>    drivers/vhost/Kconfig            |   9 +
>>>    drivers/vhost/Makefile           |   3 +
>>>    drivers/vhost/mdev.c             | 462 +++++++++++++++++++++++++++++++
>>>    drivers/vhost/vhost.c            |  39 ++-
>>>    drivers/vhost/vhost.h            |   6 +
>>>    include/linux/vfio.h             |  11 +
>>>    include/uapi/linux/vhost.h       |  10 +
>>>    include/uapi/linux/vhost_types.h |   5 +
>>>    10 files changed, 573 insertions(+), 7 deletions(-)
>>>    create mode 100644 drivers/vhost/mdev.c
>>>

^ permalink raw reply

* Re: [PATCH v3 bpf-next 00/14] samples: bpf: improve/fix cross-compilation
From: Andrii Nakryiko @ 2019-09-18  5:33 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	john fastabend, open list, Networking, bpf, clang-built-linux,
	sergei.shtylyov
In-Reply-To: <20190916105433.11404-1-ivan.khoronzhuk@linaro.org>

On Mon, Sep 16, 2019 at 4:02 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>

Thanks for these changes, they look good overall. It would be great if
someone else could test and validate that cross-compilation works not
just in your environment and generated binaries successfully run on
target machines, though...

[...]


>
> Ivan Khoronzhuk (14):
>   samples: bpf: makefile: fix HDR_PROBE "echo"
>   samples: bpf: makefile: fix cookie_uid_helper_example obj build
>   samples: bpf: makefile: use --target from cross-compile
>   samples: bpf: use own EXTRA_CFLAGS for clang commands
>   samples: bpf: makefile: use __LINUX_ARM_ARCH__ selector for arm
>   samples: bpf: makefile: drop unnecessarily inclusion for bpf_load
>   samples: bpf: add makefile.target for separate CC target build
>   samples: bpf: makefile: base target programs rules on Makefile.target
>   samples: bpf: makefile: use own flags but not host when cross compile
>   samples: bpf: makefile: use target CC environment for HDR_PROBE
>   libbpf: makefile: add C/CXX/LDFLAGS to libbpf.so and test_libpf
>     targets
>   samples: bpf: makefile: provide C/CXX/LD flags to libbpf
>   samples: bpf: makefile: add sysroot support
>   samples: bpf: README: add preparation steps and sysroot info
>

Prefixes like "samples: bpf: makefile: " are very verbose without
adding much value. We've been converging to essentially this set of
prefixes:

- "libbpf:" for libbpf changes
- "bpftool:" for bpftool changes
- "selftests/bpf:" for bpf selftests
- "samples/bpf:" for bpf samples

There is no need to prefix with "makefile: " either. Please update
your patch subjects in the next version. Thanks!

>  samples/bpf/Makefile        | 179 +++++++++++++++++++++---------------
>  samples/bpf/Makefile.target |  75 +++++++++++++++
>  samples/bpf/README.rst      |  41 ++++++++-
>  tools/lib/bpf/Makefile      |  11 ++-
>  4 files changed, 225 insertions(+), 81 deletions(-)
>  create mode 100644 samples/bpf/Makefile.target
>
> --
> 2.17.1
>

^ permalink raw reply

* Re: [PATCH v3 bpf-next 13/14] samples: bpf: makefile: add sysroot support
From: Andrii Nakryiko @ 2019-09-18  5:23 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	john fastabend, open list, Networking, bpf, clang-built-linux,
	sergei.shtylyov
In-Reply-To: <20190916105433.11404-14-ivan.khoronzhuk@linaro.org>

On Mon, Sep 16, 2019 at 4:00 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> Basically it only enables that was added by previous couple fixes.
> Sysroot contains correct libs installed and its headers ofc. Useful

Please, let's not use unnecessary abbreviations/slang. "Of course" is
not too long and is a proper English, let's stick to it.

> when working with NFC or virtual machine.
>
> Usage:
>
> clean (on demand)
>     make ARCH=arm -C samples/bpf clean
>     make ARCH=arm -C tools clean
>     make ARCH=arm clean
>
> configure and install headers:
>
>     make ARCH=arm defconfig
>     make ARCH=arm headers_install
>
> build samples/bpf:
>     make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- samples/bpf/ \
>     SYSROOT="path/to/sysroot"
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  samples/bpf/Makefile | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 133123d4c7d7..57ddf055d6c3 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -194,6 +194,11 @@ TPROGS_CFLAGS += -I$(srctree)/tools/lib/
>  TPROGS_CFLAGS += -I$(srctree)/tools/include
>  TPROGS_CFLAGS += -I$(srctree)/tools/perf
>
> +ifdef SYSROOT
> +TPROGS_CFLAGS += --sysroot=${SYSROOT}
> +TPROGS_LDFLAGS := -L${SYSROOT}/usr/lib

Please stay consistent: $() instead of ${}?

> +endif
> +
>  EXTRA_CXXFLAGS := $(TPROGS_CFLAGS)
>
>  # options not valid for C++
> --
> 2.17.1
>

^ permalink raw reply

* Re: [PATCH v3 bpf-next 12/14] samples: bpf: makefile: provide C/CXX/LD flags to libbpf
From: Andrii Nakryiko @ 2019-09-18  5:20 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	john fastabend, open list, Networking, bpf, clang-built-linux,
	sergei.shtylyov
In-Reply-To: <20190916105433.11404-13-ivan.khoronzhuk@linaro.org>

On Mon, Sep 16, 2019 at 3:58 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> In order to build libs using C/CXX/LD flags of target arch,
> provide them to libbpf make.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  samples/bpf/Makefile | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 18ec22e7b444..133123d4c7d7 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -182,8 +182,6 @@ ifdef CROSS_COMPILE
>  TPROGS_CFLAGS += -Wall
>  TPROGS_CFLAGS += -O2
>  TPROGS_CFLAGS += -fomit-frame-pointer
> -TPROGS_CFLAGS += -Wmissing-prototypes
> -TPROGS_CFLAGS += -Wstrict-prototypes
>  else
>  TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
>  TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
> @@ -196,6 +194,14 @@ TPROGS_CFLAGS += -I$(srctree)/tools/lib/
>  TPROGS_CFLAGS += -I$(srctree)/tools/include
>  TPROGS_CFLAGS += -I$(srctree)/tools/perf
>
> +EXTRA_CXXFLAGS := $(TPROGS_CFLAGS)
> +
> +# options not valid for C++
> +ifdef CROSS_COMPILE
> +$(TPROGS_CFLAGS) += -Wmissing-prototypes
> +$(TPROGS_CFLAGS) += -Wstrict-prototypes
> +endif
> +

ugh, let's really get rid of dependency on C++ compiler, as suggested
for previous patch.


>  TPROGCFLAGS_bpf_load.o += -Wno-unused-variable
>
>  TPROGS_LDLIBS                  += $(LIBBPF) -lelf
> @@ -257,7 +263,9 @@ clean:
>
>  $(LIBBPF): FORCE
>  # Fix up variables inherited from Kbuild that tools/ build system won't like
> -       $(MAKE) -C $(dir $@) RM='rm -rf' LDFLAGS= srctree=$(BPF_SAMPLES_PATH)/../../ O=
> +       $(MAKE) -C $(dir $@) RM='rm -rf' EXTRA_CFLAGS="$(TPROGS_CFLAGS)" \
> +               EXTRA_CXXFLAGS="$(EXTRA_CXXFLAGS)" LDFLAGS=$(TPROGS_LDFLAGS) \
> +               srctree=$(BPF_SAMPLES_PATH)/../../ O=
>
>  $(obj)/syscall_nrs.h:  $(obj)/syscall_nrs.s FORCE
>         $(call filechk,offsets,__SYSCALL_NRS_H__)
> --
> 2.17.1
>

^ permalink raw reply

* Re: [PATCH v3 bpf-next 11/14] libbpf: makefile: add C/CXX/LDFLAGS to libbpf.so and test_libpf targets
From: Andrii Nakryiko @ 2019-09-18  5:19 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	john fastabend, open list, Networking, bpf, clang-built-linux,
	sergei.shtylyov
In-Reply-To: <20190916105433.11404-12-ivan.khoronzhuk@linaro.org>

On Mon, Sep 16, 2019 at 4:00 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> In case of LDFLAGS and EXTRA_CC/CXX flags there is no way to pass them
> correctly to build command, for instance when --sysroot is used or
> external libraries are used, like -lelf, wich can be absent in
> toolchain. This can be used for samples/bpf cross-compiling allowing
> to get elf lib from sysroot.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  tools/lib/bpf/Makefile | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index c6f94cffe06e..bccfa556ef4e 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -94,6 +94,10 @@ else
>    CFLAGS := -g -Wall
>  endif
>
> +ifdef EXTRA_CXXFLAGS
> +  CXXFLAGS := $(EXTRA_CXXFLAGS)
> +endif
> +
>  ifeq ($(feature-libelf-mmap), 1)
>    override CFLAGS += -DHAVE_LIBELF_MMAP_SUPPORT
>  endif
> @@ -176,8 +180,9 @@ $(BPF_IN): force elfdep bpfdep
>  $(OUTPUT)libbpf.so: $(OUTPUT)libbpf.so.$(LIBBPF_VERSION)
>
>  $(OUTPUT)libbpf.so.$(LIBBPF_VERSION): $(BPF_IN)
> -       $(QUIET_LINK)$(CC) --shared -Wl,-soname,libbpf.so.$(LIBBPF_MAJOR_VERSION) \
> -                                   -Wl,--version-script=$(VERSION_SCRIPT) $^ -lelf -o $@
> +       $(QUIET_LINK)$(CC) $(LDFLAGS) \
> +               --shared -Wl,-soname,libbpf.so.$(LIBBPF_MAJOR_VERSION) \
> +               -Wl,--version-script=$(VERSION_SCRIPT) $^ -lelf -o $@
>         @ln -sf $(@F) $(OUTPUT)libbpf.so
>         @ln -sf $(@F) $(OUTPUT)libbpf.so.$(LIBBPF_MAJOR_VERSION)
>
> @@ -185,7 +190,7 @@ $(OUTPUT)libbpf.a: $(BPF_IN)
>         $(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^
>
>  $(OUTPUT)test_libbpf: test_libbpf.cpp $(OUTPUT)libbpf.a
> -       $(QUIET_LINK)$(CXX) $(INCLUDES) $^ -lelf -o $@
> +       $(QUIET_LINK)$(CXX) $(CXXFLAGS) $(LDFLAGS) $(INCLUDES) $^ -lelf -o $@

Instead of doing ifdef EXTRA_CXXFLAGS bit above, you can just include
both $(CXXFLAGS) and $(EXTRA_CXXFLAGS), which will do the right thing
(and is actually recommended my make documentation way to do this).

But actually, there is no need to use C++ compiler here,
test_libbpf.cpp can just be plain C. Do you mind renaming it to .c and
using C compiler instead?

>
>  $(OUTPUT)libbpf.pc:
>         $(QUIET_GEN)sed -e "s|@PREFIX@|$(prefix)|" \
> --
> 2.17.1
>

^ permalink raw reply

* [PATCH] drivers/net: release skb on failure
From: Navid Emamdoost @ 2019-09-18  4:45 UTC (permalink / raw)
  Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Manish Chopra,
	GR-Linux-NIC-Dev, David S. Miller, netdev, linux-kernel

In ql_run_loopback_test, ql_lb_send does not release skb when fails. So
it must be released before returning.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
index a6886cc5654c..d539b71b2a5c 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
@@ -544,8 +544,10 @@ static int ql_run_loopback_test(struct ql_adapter *qdev)
 		skb_put(skb, size);
 		ql_create_lb_frame(skb, size);
 		rc = ql_lb_send(skb, qdev->ndev);
-		if (rc != NETDEV_TX_OK)
+		if (rc != NETDEV_TX_OK) {
+			dev_kfree_skb_any(skb);
 			return -EPIPE;
+		}
 		atomic_inc(&qdev->lb_count);
 	}
 	/* Give queue time to settle before testing results. */
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH v3 bpf-next 10/14] samples: bpf: makefile: use target CC environment for HDR_PROBE
From: Andrii Nakryiko @ 2019-09-18  4:20 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	john fastabend, open list, Networking, bpf, clang-built-linux,
	sergei.shtylyov
In-Reply-To: <20190916105433.11404-11-ivan.khoronzhuk@linaro.org>

On Mon, Sep 16, 2019 at 3:59 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> No need in hacking HOSTCC to be cross-compiler any more, so drop
> this trick and use target CC for HDR_PROBE.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>


[...]

^ permalink raw reply

* Re: [PATCH 3/6] Timer: expose monotonic clock and counter value
From: Richard Cochran @ 2019-09-18  3:42 UTC (permalink / raw)
  To: Jianyong Wu
  Cc: netdev, pbonzini, sean.j.christopherson, maz, Mark.Rutland,
	Will.Deacon, suzuki.poulose, linux-kernel, Steve.Capper, Kaly.Xin,
	justin.he, nd, linux-arm-kernel
In-Reply-To: <20190917112430.45680-4-jianyong.wu@arm.com>

On Tue, Sep 17, 2019 at 07:24:27AM -0400, Jianyong Wu wrote:
> A number of PTP drivers (such as ptp-kvm) are assuming what the
> current clock source is, which could lead to interesting effects on
> systems where the clocksource can change depending on external events.
> 
> For this purpose, add a new API that retrives both the current
> monotonic clock as well as its counter value.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  include/linux/timekeeping.h |  3 +++
>  kernel/time/timekeeping.c   | 13 +++++++++++++
>  2 files changed, 16 insertions(+)

For core time keeping changes, you must CC lkml, tglx, and John Stultz.

Thanks,
Richard

^ permalink raw reply

* RE: [PATCH 3/6] Timer: expose monotonic clock and counter value
From: Jianyong Wu (Arm Technology China) @ 2019-09-18  2:50 UTC (permalink / raw)
  To: Marc Zyngier, netdev@vger.kernel.org, pbonzini@redhat.com,
	sean.j.christopherson@intel.com, richardcochran@gmail.com,
	Mark Rutland, Will Deacon, Suzuki Poulose
  Cc: linux-kernel@vger.kernel.org, Steve Capper,
	Kaly Xin (Arm Technology China), Justin He (Arm Technology China),
	nd, linux-arm-kernel@lists.infradead.org
In-Reply-To: <ad38f692-a7c4-34e0-8236-ebd2d237bd93@kernel.org>

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Wednesday, September 18, 2019 1:10 AM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>
> Cc: linux-kernel@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>;
> Kaly Xin (Arm Technology China) <Kaly.Xin@arm.com>; Justin He (Arm
> Technology China) <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 3/6] Timer: expose monotonic clock and counter value
> 
> On 17/09/2019 12:24, Jianyong Wu wrote:
> > A number of PTP drivers (such as ptp-kvm) are assuming what the
> > current clock source is, which could lead to interesting effects on
> > systems where the clocksource can change depending on external events.
> >
> > For this purpose, add a new API that retrives both the current
> > monotonic clock as well as its counter value.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> 
> There must be something wrong with the way you've taken this patch in your
> tree. My authorship is gone (not that I deeply care about it, but it is good
> practice to keep attributions), and the subject line has been rewritten.
> 
> I'd appreciate it if you could fix this in a future revision of this series. For
> reference, the original patch is here[1].
> 

Sorry for "steal" your patch, I'm not familiar with it and neglect this important change.
I just copy this patch from your email and add the subject myself.
I will fix all of them later in v3.

> > ---
> >  include/linux/timekeeping.h |  3 +++
> >  kernel/time/timekeeping.c   | 13 +++++++++++++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> > index a8ab0f143ac4..a5389adaa8bc 100644
> > --- a/include/linux/timekeeping.h
> > +++ b/include/linux/timekeeping.h
> > @@ -247,6 +247,9 @@ extern int get_device_system_crosststamp(
> >  			struct system_time_snapshot *history,
> >  			struct system_device_crosststamp *xtstamp);
> >
> > +/* Obtain current monotonic clock and its counter value  */ extern
> > +void get_current_counterval(struct system_counterval_t *sc);
> > +
> >  /*
> >   * Simultaneously snapshot realtime and monotonic raw clocks
> >   */
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index 44b726bab4bd..07a0969625b1 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1098,6 +1098,19 @@ static bool cycle_between(u64 before, u64 test,
> u64 after)
> >  	return false;
> >  }
> >
> > +/**
> > + * get_current_counterval - Snapshot the current clocksource and counter
> value
> > + * @sc:	Pointer to a struct containing the current clocksource and its
> value
> > + */
> > +void get_current_counterval(struct system_counterval_t *sc) {
> > +	struct timekeeper *tk = &tk_core.timekeeper;
> > +
> > +	sc->cs = READ_ONCE(tk->tkr_mono.clock);
> > +	sc->cycles = sc->cs->read(sc->cs);
> > +}
> > +EXPORT_SYMBOL_GPL(get_current_counterval);
> 
> This export wasn't in my original patch. I guess you need it because your ptp
> driver builds as a module? It'd be good to mention it in the commit log.
> 
Yeah, ptp_kvm will be a module, so export is necessary. I will mention this change
in commit log.

Thanks
Jianyong Wu

> > +
> >  /**
> >   * get_device_system_crosststamp - Synchronously capture
> system/device timestamp
> >   * @get_time_fn:	Callback to get simultaneous device time and
> >
> 
> Thanks,
> 
> 	M.
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-
> platforms.git/commit/?h=timer/counterval&id=a6e8abce025691b6a55e1c195
> 878d7f76bfeb9d1
> --
> Jazz is not dead, it just smells funny...

^ permalink raw reply

* Re: [PATCH net-next] ixgbe: Use memzero_explicit directly in crypto cases
From: zhong jiang @ 2019-09-18  2:43 UTC (permalink / raw)
  To: zhong jiang
  Cc: jakub.kicinski, davem, anna.schumaker, trond.myklebust, netdev,
	linux-kernel
In-Reply-To: <1568774195-8677-1-git-send-email-zhongjiang@huawei.com>

On 2019/9/18 10:36, zhong jiang wrote:
> In general, Use kzfree() to replace memset() + kfree() is feasible and
> resonable.  But It's btter to use memzero_explicit() to replace memset()
> in crypto cases.
s/btter/better/,  will repost.   sorry for that.

Thanks,
zhong jiang
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index 113f608..7e4f32f 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -960,9 +960,11 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
>  	return 0;
>  
>  err_aead:
> -	kzfree(xs->aead);
> +	memzero_explicit(xs->aead, sizeof(*xs->aead));
> +	kfree(xs->aead);
>  err_xs:
> -	kzfree(xs);
> +	memzero_explicit(xs, sizeof(*xs));
> +	kfree(xs);
>  err_out:
>  	msgbuf[1] = err;
>  	return err;
> @@ -1047,7 +1049,8 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
>  	ixgbe_ipsec_del_sa(xs);
>  
>  	/* remove the xs that was made-up in the add request */
> -	kzfree(xs);
> +	memzero_explicit(xs, sizeof(*xs));
> +	kfree(xs);
>  
>  	return 0;
>  }



^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox