rtnetlink: preparation patches for further rtnl lock pushdown/removal
Subsystems (3)
v3 cover rtnetlink: preparation patches for further rtnl lock pushdown/removal
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.
PATCH v3 1/6 selftests: rtnetlink.sh: add rudimentary vrf test
---
Changes since v1: indent all lines with tabs, not spaces
tools/testing/selftests/net/rtnetlink.sh | 42 ++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
tools/testing/selftests/net/rtnetlink.sh +42 -0
@@ -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 }
Signature · 1 line
2.13.5
PATCH v3 2/6 rtnetlink: add helper to put master and link ifindexes
In reply to Florian Westphalrtnl_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.
---
no changes since v2.
net/core/rtnetlink.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)
net/core/rtnetlink.c +27 -5
@@ -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)) ||
Signature · 1 line
2.13.5
PATCH v3 3/6 rtnetlink: add helper to dump ifalias
In reply to Florian Westphal---
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(-)
net/core/rtnetlink.c +9 -2
@@ -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))
Signature · 1 line
2.13.5
Re: [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias
Quoted 14 lines · Florian Westphal
Reviewed-by David Ahern <[email protected]>Signed-off-by Florian Westphal <[email protected]>---
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(-)net/core/rtnetlink.c +0
-0
net/core/rtnetlink.c static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev) · line 1335
Quoted 4 lines
net/core/rtnetlink.c +1
-0@@ -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 )
net/core/rtnetlink.c static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev) · line 1342
Quoted 7 lines
net/core/rtnetlink.c +7
-0+{ +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...
Re: [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias
Quoted 26 lines · Eric Dumazet
Quoted 14 lines · Florian Westphal
Reviewed-by David Ahern <[email protected]>Signed-off-by Florian Westphal <[email protected]>---
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(-)net/core/rtnetlink.c +0
-0net/core/rtnetlink.c static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev) · line 1335
Quoted 4 lines
net/core/rtnetlink.c +1
-0@@ -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)
Quoted 10 lines
net/core/rtnetlink.c static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev) · line 1342
Quoted 7 lines
net/core/rtnetlink.c +7
-0+{ +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.
--- 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(-)
include/linux/netdevice.h +2 -1
@@ -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 );
net/core/dev.c +59 -11
100 lines not on this page · raw message
net/core/net-sysfs.c +5 -9
@@ -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); }
net/core/rtnetlink.c +4 -3
@@ -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,
Signature · 1 line
2.13.5
David Miller
maintainerRe: [PATCH net-next v3 3/6] rtnetlink: add helper to dump ifalias
In reply to Florian WestphalFrom: Florian Westphal <[email protected]>
Date: Sat, 23 Sep 2017 21:26:33 +0200
net/core/rtnetlink.c static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev) · line 1335
Quoted 5 lines
net/core/rtnetlink.c +1
-0@@ -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.
PATCH v3 4/6 rtnetlink: add helpers to dump vf information
In reply to Florian Westphalsimilar to earlier patches, split out more parts of this function to better see what is happening and where we assume rtnl is locked.
---
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(-)
net/core/rtnetlink.c +31 -19
@@ -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;
Signature · 1 line
2.13.5
PATCH v3 5/6 rtnetlink: add helpers to dump netnsid information
In reply to Florian Westphal---
Changes since v2:
this hunk was part of patch #4.
net/core/rtnetlink.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)
net/core/rtnetlink.c +19 -11
@@ -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;
Signature · 1 line
2.13.5
PATCH v3 6/6 rtnetlink: rtnl_have_link_slave_info doesn't need rtnl
In reply to Florian Westphalit can be switched to rcu.
---
Changes since v2: remove ASSERT_RTNL.
net/core/rtnetlink.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
net/core/rtnetlink.c +7 -3
@@ -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,
Signature · 1 line
2.13.5