All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v3 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal
@ 2017-09-23 19:26 Florian Westphal
  2017-09-23 19:26 ` [PATCH net-next v3 1/6] selftests: rtnetlink.sh: add rudimentary vrf test Florian Westphal
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Florian Westphal @ 2017-09-23 19:26 UTC (permalink / raw)
  To: netdev

First patch adds a rudimentary vrf test case.

Remaining patches split large rtnl_fill_ifinfo into smaller chunks
to better see which parts

1. require rtnl
2. do not require it at all
3. rely on rtnl locking now but could be converted

I removed all the ASSERT_RTNL spots that v1 and v2 added,
i will keep that back in my working branch since those
are just 'todo' markers for myself.

Eric Dumazet pointed out that qdiscs are now freed directly without
call_rcu so I dropped the patch that made this assumption from the series.

As the remaining patches did not see major changes vs. v2 I retained
all reviewed/acked-by tags from David Ahern.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net-next v3 1/6] selftests: rtnetlink.sh: add rudimentary vrf test
  2017-09-23 19:26 [PATCH net-next v3 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal Florian Westphal
@ 2017-09-23 19:26 ` Florian Westphal
  2017-09-23 19:26 ` [PATCH net-next v3 2/6] rtnetlink: add helper to put master and link ifindexes Florian Westphal
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2017-09-23 19:26 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal

Acked-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Changes since v1: indent all lines with tabs, not spaces

 tools/testing/selftests/net/rtnetlink.sh | 42 ++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 4b48de565cae..a048f7a5f94c 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -291,6 +291,47 @@ kci_test_ifalias()
 	echo "PASS: set ifalias $namewant for $devdummy"
 }
 
+kci_test_vrf()
+{
+	vrfname="test-vrf"
+	ret=0
+
+	ip link show type vrf 2>/dev/null
+	if [ $? -ne 0 ]; then
+		echo "SKIP: vrf: iproute2 too old"
+		return 0
+	fi
+
+	ip link add "$vrfname" type vrf table 10
+	check_err $?
+	if [ $ret -ne 0 ];then
+		echo "FAIL: can't add vrf interface, skipping test"
+		return 0
+	fi
+
+	ip -br link show type vrf | grep -q "$vrfname"
+	check_err $?
+	if [ $ret -ne 0 ];then
+		echo "FAIL: created vrf device not found"
+		return 1
+	fi
+
+	ip link set dev "$vrfname" up
+	check_err $?
+
+	ip link set dev "$devdummy" master "$vrfname"
+	check_err $?
+	ip link del dev "$vrfname"
+	check_err $?
+
+	if [ $ret -ne 0 ];then
+		echo "FAIL: vrf"
+		return 1
+	fi
+
+	echo "PASS: vrf"
+}
+
 kci_test_rtnl()
 {
 	kci_add_dummy
@@ -306,6 +347,7 @@ kci_test_rtnl()
 	kci_test_bridge
 	kci_test_addrlabel
 	kci_test_ifalias
+	kci_test_vrf
 
 	kci_del_dummy
 }
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v3 2/6] rtnetlink: add helper to put master and link ifindexes
  2017-09-23 19:26 [PATCH net-next v3 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal Florian Westphal
  2017-09-23 19:26 ` [PATCH net-next v3 1/6] selftests: rtnetlink.sh: add rudimentary vrf test Florian Westphal
@ 2017-09-23 19:26 ` Florian Westphal
  2017-09-23 19:26 ` [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias Florian Westphal
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2017-09-23 19:26 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal

rtnl_fill_ifinfo currently requires caller to hold the rtnl mutex.
Unfortunately the function is quite large which makes it harder to see
which spots require the lock, which spots assume it and which ones could
do without.

Add helpers to factor out the ifindex dumping, one can use rcu to avoid
rtnl dependency.

Reviewed-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 no changes since v2.
 net/core/rtnetlink.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a78fd61da0ec..c801212ee40e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1307,6 +1307,31 @@ static u32 rtnl_get_event(unsigned long event)
 	return rtnl_event_type;
 }
 
+static int put_master_ifindex(struct sk_buff *skb, struct net_device *dev)
+{
+	const struct net_device *upper_dev;
+	int ret = 0;
+
+	rcu_read_lock();
+
+	upper_dev = netdev_master_upper_dev_get_rcu(dev);
+	if (upper_dev)
+		ret = nla_put_u32(skb, IFLA_MASTER, upper_dev->ifindex);
+
+	rcu_read_unlock();
+	return ret;
+}
+
+static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)
+{
+	int ifindex = dev_get_iflink(dev);
+
+	if (dev->ifindex == ifindex)
+		return 0;
+
+	return nla_put_u32(skb, IFLA_LINK, ifindex);
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags, u32 ext_filter_mask,
@@ -1316,7 +1341,6 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	struct nlmsghdr *nlh;
 	struct nlattr *af_spec;
 	struct rtnl_af_ops *af_ops;
-	struct net_device *upper_dev = netdev_master_upper_dev_get(dev);
 
 	ASSERT_RTNL();
 	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifm), flags);
@@ -1345,10 +1369,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 #ifdef CONFIG_RPS
 	    nla_put_u32(skb, IFLA_NUM_RX_QUEUES, dev->num_rx_queues) ||
 #endif
-	    (dev->ifindex != dev_get_iflink(dev) &&
-	     nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev))) ||
-	    (upper_dev &&
-	     nla_put_u32(skb, IFLA_MASTER, upper_dev->ifindex)) ||
+	    nla_put_iflink(skb, dev) ||
+	    put_master_ifindex(skb, dev) ||
 	    nla_put_u8(skb, IFLA_CARRIER, netif_carrier_ok(dev)) ||
 	    (dev->qdisc &&
 	     nla_put_string(skb, IFLA_QDISC, dev->qdisc->ops->id)) ||
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias
  2017-09-23 19:26 [PATCH net-next v3 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal Florian Westphal
  2017-09-23 19:26 ` [PATCH net-next v3 1/6] selftests: rtnetlink.sh: add rudimentary vrf test Florian Westphal
  2017-09-23 19:26 ` [PATCH net-next v3 2/6] rtnetlink: add helper to put master and link ifindexes Florian Westphal
@ 2017-09-23 19:26 ` Florian Westphal
  2017-09-23 21:04   ` Eric Dumazet
  2017-09-26  3:34   ` David Miller
  2017-09-23 19:26 ` [PATCH net-next v3 4/6] rtnetlink: add helpers to dump vf information Florian Westphal
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 10+ messages in thread
From: Florian Westphal @ 2017-09-23 19:26 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal

Reviewed-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Changes since v3: don't add rtnl assertion, I placed the assertion
 in my working tree instead as a reminder.

 net/core/rtnetlink.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index c801212ee40e..47c17c3de79a 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1332,6 +1332,14 @@ static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)
 	return nla_put_u32(skb, IFLA_LINK, ifindex);
 }
 
+static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)
+{
+	if (dev->ifalias)
+		return nla_put_string(skb, IFLA_IFALIAS, dev->ifalias);
+
+	return 0;
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags, u32 ext_filter_mask,
@@ -1374,8 +1382,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	    nla_put_u8(skb, IFLA_CARRIER, netif_carrier_ok(dev)) ||
 	    (dev->qdisc &&
 	     nla_put_string(skb, IFLA_QDISC, dev->qdisc->ops->id)) ||
-	    (dev->ifalias &&
-	     nla_put_string(skb, IFLA_IFALIAS, dev->ifalias)) ||
+	    nla_put_ifalias(skb, dev) ||
 	    nla_put_u32(skb, IFLA_CARRIER_CHANGES,
 			atomic_read(&dev->carrier_changes)) ||
 	    nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down))
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v3 4/6] rtnetlink: add helpers to dump vf information
  2017-09-23 19:26 [PATCH net-next v3 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal Florian Westphal
                   ` (2 preceding siblings ...)
  2017-09-23 19:26 ` [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias Florian Westphal
@ 2017-09-23 19:26 ` Florian Westphal
  2017-09-23 19:26 ` [PATCH net-next v3 5/6] rtnetlink: add helpers to dump netnsid information Florian Westphal
  2017-09-23 19:26 ` [PATCH net-next v3 6/6] rtnetlink: rtnl_have_link_slave_info doesn't need rtnl Florian Westphal
  5 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2017-09-23 19:26 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal

similar to earlier patches, split out more parts of this function to
better see what is happening and where we assume rtnl is locked.

Reviewed-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 changes since v2: split this patch into two, last submission also
 added netnsid helper, which was moved to next patch.

 net/core/rtnetlink.c | 50 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 47c17c3de79a..625342d27c44 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1211,6 +1211,36 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
 	return -EMSGSIZE;
 }
 
+static noinline_for_stack int rtnl_fill_vf(struct sk_buff *skb,
+					   struct net_device *dev,
+					   u32 ext_filter_mask)
+{
+	struct nlattr *vfinfo;
+	int i, num_vfs;
+
+	if (!dev->dev.parent || ((ext_filter_mask & RTEXT_FILTER_VF) == 0))
+		return 0;
+
+	num_vfs = dev_num_vf(dev->dev.parent);
+	if (nla_put_u32(skb, IFLA_NUM_VF, num_vfs))
+		return -EMSGSIZE;
+
+	if (!dev->netdev_ops->ndo_get_vf_config)
+		return 0;
+
+	vfinfo = nla_nest_start(skb, IFLA_VFINFO_LIST);
+	if (!vfinfo)
+		return -EMSGSIZE;
+
+	for (i = 0; i < num_vfs; i++) {
+		if (rtnl_fill_vfinfo(skb, dev, i, vfinfo))
+			return -EMSGSIZE;
+	}
+
+	nla_nest_end(skb, vfinfo);
+	return 0;
+}
+
 static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
 {
 	struct rtnl_link_ifmap map;
@@ -1414,27 +1444,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	if (rtnl_fill_stats(skb, dev))
 		goto nla_put_failure;
 
-	if (dev->dev.parent && (ext_filter_mask & RTEXT_FILTER_VF) &&
-	    nla_put_u32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent)))
+	if (rtnl_fill_vf(skb, dev, ext_filter_mask))
 		goto nla_put_failure;
 
-	if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent &&
-	    ext_filter_mask & RTEXT_FILTER_VF) {
-		int i;
-		struct nlattr *vfinfo;
-		int num_vfs = dev_num_vf(dev->dev.parent);
-
-		vfinfo = nla_nest_start(skb, IFLA_VFINFO_LIST);
-		if (!vfinfo)
-			goto nla_put_failure;
-		for (i = 0; i < num_vfs; i++) {
-			if (rtnl_fill_vfinfo(skb, dev, i, vfinfo))
-				goto nla_put_failure;
-		}
-
-		nla_nest_end(skb, vfinfo);
-	}
-
 	if (rtnl_port_fill(skb, dev, ext_filter_mask))
 		goto nla_put_failure;
 
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v3 5/6] rtnetlink: add helpers to dump netnsid information
  2017-09-23 19:26 [PATCH net-next v3 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal Florian Westphal
                   ` (3 preceding siblings ...)
  2017-09-23 19:26 ` [PATCH net-next v3 4/6] rtnetlink: add helpers to dump vf information Florian Westphal
@ 2017-09-23 19:26 ` Florian Westphal
  2017-09-23 19:26 ` [PATCH net-next v3 6/6] rtnetlink: rtnl_have_link_slave_info doesn't need rtnl Florian Westphal
  5 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2017-09-23 19:26 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal

Reviewed-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Changes since v2:
 this hunk was part of patch #4.

 net/core/rtnetlink.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 625342d27c44..e858a2b48d7e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1370,6 +1370,23 @@ static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static noinline int rtnl_fill_link_netnsid(struct sk_buff *skb,
+					   const struct net_device *dev)
+{
+	if (dev->rtnl_link_ops && dev->rtnl_link_ops->get_link_net) {
+		struct net *link_net = dev->rtnl_link_ops->get_link_net(dev);
+
+		if (!net_eq(dev_net(dev), link_net)) {
+			int id = peernet2id_alloc(dev_net(dev), link_net);
+
+			if (nla_put_s32(skb, IFLA_LINK_NETNSID, id))
+				return -EMSGSIZE;
+		}
+	}
+
+	return 0;
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags, u32 ext_filter_mask,
@@ -1458,17 +1475,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			goto nla_put_failure;
 	}
 
-	if (dev->rtnl_link_ops &&
-	    dev->rtnl_link_ops->get_link_net) {
-		struct net *link_net = dev->rtnl_link_ops->get_link_net(dev);
-
-		if (!net_eq(dev_net(dev), link_net)) {
-			int id = peernet2id_alloc(dev_net(dev), link_net);
-
-			if (nla_put_s32(skb, IFLA_LINK_NETNSID, id))
-				goto nla_put_failure;
-		}
-	}
+	if (rtnl_fill_link_netnsid(skb, dev))
+		goto nla_put_failure;
 
 	if (!(af_spec = nla_nest_start(skb, IFLA_AF_SPEC)))
 		goto nla_put_failure;
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v3 6/6] rtnetlink: rtnl_have_link_slave_info doesn't need rtnl
  2017-09-23 19:26 [PATCH net-next v3 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal Florian Westphal
                   ` (4 preceding siblings ...)
  2017-09-23 19:26 ` [PATCH net-next v3 5/6] rtnetlink: add helpers to dump netnsid information Florian Westphal
@ 2017-09-23 19:26 ` Florian Westphal
  5 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2017-09-23 19:26 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal

it can be switched to rcu.

Reviewed-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Changes since v2: remove ASSERT_RTNL.

 net/core/rtnetlink.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index e858a2b48d7e..c69451964a44 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -522,11 +522,15 @@ static size_t rtnl_link_get_af_size(const struct net_device *dev,
 static bool rtnl_have_link_slave_info(const struct net_device *dev)
 {
 	struct net_device *master_dev;
+	bool ret = false;
 
-	master_dev = netdev_master_upper_dev_get((struct net_device *) dev);
+	rcu_read_lock();
+
+	master_dev = netdev_master_upper_dev_get_rcu((struct net_device *)dev);
 	if (master_dev && master_dev->rtnl_link_ops)
-		return true;
-	return false;
+		ret = true;
+	rcu_read_unlock();
+	return ret;
 }
 
 static int rtnl_link_slave_info_fill(struct sk_buff *skb,
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias
  2017-09-23 19:26 ` [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias Florian Westphal
@ 2017-09-23 21:04   ` Eric Dumazet
  2017-09-23 21:21     ` Florian Westphal
  2017-09-26  3:34   ` David Miller
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2017-09-23 21:04 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netdev

On Sat, 2017-09-23 at 21:26 +0200, Florian Westphal wrote:
> Reviewed-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Changes since v3: don't add rtnl assertion, I placed the assertion
>  in my working tree instead as a reminder.
> 
>  net/core/rtnetlink.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index c801212ee40e..47c17c3de79a 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1332,6 +1332,14 @@ static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)
>  	return nla_put_u32(skb, IFLA_LINK, ifindex);
>  }
>  
> +static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)


Why noinline here ?

This function does not use stack at all (and that would call for
noinline_for_stack )

> +{
> +	if (dev->ifalias)
> +		return nla_put_string(skb, IFLA_IFALIAS, dev->ifalias);
> +
> +	return 0;
> +}
> +

I really do not see the point of not making this RCU aware right away,
or at least make it in the same patch series...

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias
  2017-09-23 21:04   ` Eric Dumazet
@ 2017-09-23 21:21     ` Florian Westphal
  0 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2017-09-23 21:21 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Florian Westphal, netdev

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sat, 2017-09-23 at 21:26 +0200, Florian Westphal wrote:
> > Reviewed-by: David Ahern <dsahern@gmail.com>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  Changes since v3: don't add rtnl assertion, I placed the assertion
> >  in my working tree instead as a reminder.
> > 
> >  net/core/rtnetlink.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index c801212ee40e..47c17c3de79a 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1332,6 +1332,14 @@ static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)
> >  	return nla_put_u32(skb, IFLA_LINK, ifindex);
> >  }
> >  
> > +static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)
> 
> 
> Why noinline here ?
> 
> This function does not use stack at all (and that would call for
> noinline_for_stack )
> 
> > +{
> > +	if (dev->ifalias)
> > +		return nla_put_string(skb, IFLA_IFALIAS, dev->ifalias);
> > +
> > +	return 0;
> > +}
> > +
> 
> I really do not see the point of not making this RCU aware right away,
> or at least make it in the same patch series...

I saw no point to mix these refactoring with actual change :-|

(and it doesn't help either as-is with netlink dumping, only sysfs path can
 elide rtnl).

Subject: [PATCH net-next] net: core: decouple ifalias get/set from rtnl lock

Device alias can be set by either rtnetlink (rtnl is held) or sysfs.

rtnetlink holds rtnl mutex, sysfs acquires it for this purpose.
Add a new mutex for it plus a seqcount to get a consistent snapshot
of the alias buffer.

This allows the sysfs path to not take rtnl and would later allow
to not hold it when dumping ifalias.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/linux/netdevice.h |  3 +-
 net/core/dev.c            | 70 +++++++++++++++++++++++++++++++++++++++--------
 net/core/net-sysfs.c      | 14 ++++------
 net/core/rtnetlink.c      |  7 +++--
 4 files changed, 70 insertions(+), 24 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f535779d9dc1..0bcedb498f6f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1632,7 +1632,7 @@ enum netdev_priv_flags {
 struct net_device {
 	char			name[IFNAMSIZ];
 	struct hlist_node	name_hlist;
-	char 			*ifalias;
+	char 			__rcu *ifalias;
 	/*
 	 *	I/O specific fields
 	 *	FIXME: Merge these and struct ifmap into one
@@ -3275,6 +3275,7 @@ void __dev_notify_flags(struct net_device *, unsigned int old_flags,
 			unsigned int gchanges);
 int dev_change_name(struct net_device *, const char *);
 int dev_set_alias(struct net_device *, const char *, size_t);
+int dev_get_alias(const struct net_device *, char *, size_t);
 int dev_change_net_namespace(struct net_device *, struct net *, const char *);
 int __dev_set_mtu(struct net_device *, int);
 int dev_set_mtu(struct net_device *, int);
diff --git a/net/core/dev.c b/net/core/dev.c
index 97abddd9039a..92d87b4a2db1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -188,6 +188,9 @@ static struct napi_struct *napi_by_id(unsigned int napi_id);
 DEFINE_RWLOCK(dev_base_lock);
 EXPORT_SYMBOL(dev_base_lock);
 
+static DEFINE_MUTEX(ifalias_mutex);
+static seqcount_t ifalias_rename_seq;
+
 /* protects napi_hash addition/deletion and napi_gen_id */
 static DEFINE_SPINLOCK(napi_hash_lock);
 
@@ -1265,29 +1268,72 @@ int dev_change_name(struct net_device *dev, const char *newname)
  */
 int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
 {
-	char *new_ifalias;
-
-	ASSERT_RTNL();
+	char *new_ifalias, *old_ifalias;
 
 	if (len >= IFALIASZ)
 		return -EINVAL;
 
+	mutex_lock(&ifalias_mutex);
+
+	old_ifalias = rcu_dereference_protected(dev->ifalias,
+						mutex_is_locked(&ifalias_mutex));
 	if (!len) {
-		kfree(dev->ifalias);
-		dev->ifalias = NULL;
-		return 0;
+		RCU_INIT_POINTER(dev->ifalias, NULL);
+		goto out;
 	}
 
-	new_ifalias = krealloc(dev->ifalias, len + 1, GFP_KERNEL);
-	if (!new_ifalias)
+	new_ifalias = __krealloc(old_ifalias, len + 1, GFP_KERNEL);
+	if (!new_ifalias) {
+		mutex_unlock(&ifalias_mutex);
 		return -ENOMEM;
-	dev->ifalias = new_ifalias;
-	memcpy(dev->ifalias, alias, len);
-	dev->ifalias[len] = 0;
+	}
 
+	if (new_ifalias == old_ifalias) {
+		write_seqcount_begin(&ifalias_rename_seq);
+		memcpy(new_ifalias, alias, len);
+		new_ifalias[len] = 0;
+		write_seqcount_end(&ifalias_rename_seq);
+		mutex_unlock(&ifalias_mutex);
+		return len;
+	}
+
+	memcpy(new_ifalias, alias, len);
+	new_ifalias[len] = 0;
+
+	rcu_assign_pointer(dev->ifalias, new_ifalias);
+out:
+	mutex_unlock(&ifalias_mutex);
+	if (old_ifalias) {
+		synchronize_net();
+		kfree(old_ifalias);
+	}
 	return len;
 }
 
+int dev_get_alias(const struct net_device *dev, char *alias, size_t len)
+{
+	unsigned int seq;
+	int ret;
+
+	for (;;) {
+		const char *name;
+
+		ret = 0;
+		rcu_read_lock();
+		name = rcu_dereference(dev->ifalias);
+		seq = raw_seqcount_begin(&ifalias_rename_seq);
+		if (name)
+			ret = snprintf(alias, len, "%s", name);
+		rcu_read_unlock();
+
+		if (!read_seqcount_retry(&ifalias_rename_seq, seq))
+			break;
+
+		cond_resched();
+	}
+
+	return ret;
+}
 
 /**
  *	netdev_features_change - device changes features
@@ -8749,6 +8795,8 @@ static int __init net_dev_init(void)
 				       NULL, dev_cpu_dead);
 	WARN_ON(rc < 0);
 	rc = 0;
+
+	seqcount_init(&ifalias_rename_seq);
 out:
 	return rc;
 }
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 927a6dcbad96..530de7996d65 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -391,10 +391,7 @@ static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
 	if (len >  0 && buf[len - 1] == '\n')
 		--count;
 
-	if (!rtnl_trylock())
-		return restart_syscall();
 	ret = dev_set_alias(netdev, buf, count);
-	rtnl_unlock();
 
 	return ret < 0 ? ret : len;
 }
@@ -403,13 +400,12 @@ static ssize_t ifalias_show(struct device *dev,
 			    struct device_attribute *attr, char *buf)
 {
 	const struct net_device *netdev = to_net_dev(dev);
+	char tmp[IFALIASZ];
 	ssize_t ret = 0;
 
-	if (!rtnl_trylock())
-		return restart_syscall();
-	if (netdev->ifalias)
-		ret = sprintf(buf, "%s\n", netdev->ifalias);
-	rtnl_unlock();
+	ret = dev_get_alias(netdev, tmp, sizeof(tmp));
+	if (ret > 0)
+		ret = sprintf(buf, "%s\n", tmp);
 	return ret;
 }
 static DEVICE_ATTR_RW(ifalias);
@@ -1488,7 +1484,7 @@ static void netdev_release(struct device *d)
 
 	BUG_ON(dev->reg_state != NETREG_RELEASED);
 
-	kfree(dev->ifalias);
+	dev_set_alias(dev, "", 0);
 	netdev_freemem(dev);
 }
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index c69451964a44..bab108ced7d8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1368,10 +1368,11 @@ static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)
 
 static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)
 {
-	if (dev->ifalias)
-		return nla_put_string(skb, IFLA_IFALIAS, dev->ifalias);
+	char buf[IFALIASZ];
+	int ret;
 
-	return 0;
+	ret = dev_get_alias(dev, buf, sizeof(buf));
+	return ret > 0 ? nla_put_string(skb, IFLA_IFALIAS, buf) : 0;
 }
 
 static noinline int rtnl_fill_link_netnsid(struct sk_buff *skb,
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias
  2017-09-23 19:26 ` [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias Florian Westphal
  2017-09-23 21:04   ` Eric Dumazet
@ 2017-09-26  3:34   ` David Miller
  1 sibling, 0 replies; 10+ messages in thread
From: David Miller @ 2017-09-26  3:34 UTC (permalink / raw)
  To: fw; +Cc: netdev

From: Florian Westphal <fw@strlen.de>
Date: Sat, 23 Sep 2017 21:26:33 +0200

> @@ -1332,6 +1332,14 @@ static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)
>  	return nla_put_u32(skb, IFLA_LINK, ifindex);
>  }
>  
> +static noinline int nla_put_ifalias(struct sk_buff *skb, struct net_device *dev)

Please do use inline annoations in foo.c files unless there is a very strong
specific need.

Thank you.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-09-26  3:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-23 19:26 [PATCH net-next v3 0/6] rtnetlink: preparation patches for further rtnl lock pushdown/removal Florian Westphal
2017-09-23 19:26 ` [PATCH net-next v3 1/6] selftests: rtnetlink.sh: add rudimentary vrf test Florian Westphal
2017-09-23 19:26 ` [PATCH net-next v3 2/6] rtnetlink: add helper to put master and link ifindexes Florian Westphal
2017-09-23 19:26 ` [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias Florian Westphal
2017-09-23 21:04   ` Eric Dumazet
2017-09-23 21:21     ` Florian Westphal
2017-09-26  3:34   ` David Miller
2017-09-23 19:26 ` [PATCH net-next v3 4/6] rtnetlink: add helpers to dump vf information Florian Westphal
2017-09-23 19:26 ` [PATCH net-next v3 5/6] rtnetlink: add helpers to dump netnsid information Florian Westphal
2017-09-23 19:26 ` [PATCH net-next v3 6/6] rtnetlink: rtnl_have_link_slave_info doesn't need rtnl Florian Westphal

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.