diff mbox series

[RFC,net-next,9/9] net-sysfs: remove the use of rtnl_trylock/restart_syscall

Message ID 20210928125500.167943-10-atenart@kernel.org (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series Userspace spinning on net-sysfs access | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 3 maintainers not CCed: weiwan@google.com yebin10@huawei.com alexanderduyck@fb.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 183 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/header_inline success Link

Commit Message

Antoine Tenart Sept. 28, 2021, 12:55 p.m. UTC 
The ABBA deadlock avoided by using rtnl_trylock and restart_syscall was
fixed in previous commits, we can now remove the use of this
trylock/restart logic and have net-sysfs operations not spinning when
rtnl is already taken.

Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 net/core/net-sysfs.c | 95 +++++++-------------------------------------
 1 file changed, 14 insertions(+), 81 deletions(-)

Comments

Michal Hocko Oct. 6, 2021, 6:45 a.m. UTC | #1 
On Tue 28-09-21 14:55:00, Antoine Tenart wrote:
> The ABBA deadlock avoided by using rtnl_trylock and restart_syscall was
> fixed in previous commits, we can now remove the use of this
> trylock/restart logic and have net-sysfs operations not spinning when
> rtnl is already taken.

Shouldn't those lock be interruptible or killable at least? As mentioned
in other reply we are seeing multiple a contention on some sysfs file
because mlx driver tends to do some heavy lifting in its speed callback
so it would be great to be able to interact with waiters during that
time.
Antoine Tenart Oct. 6, 2021, 8:03 a.m. UTC | #2 
Quoting Michal Hocko (2021-10-06 08:45:58)
> On Tue 28-09-21 14:55:00, Antoine Tenart wrote:
> > The ABBA deadlock avoided by using rtnl_trylock and restart_syscall was
> > fixed in previous commits, we can now remove the use of this
> > trylock/restart logic and have net-sysfs operations not spinning when
> > rtnl is already taken.
> 
> Shouldn't those lock be interruptible or killable at least? As mentioned
> in other reply we are seeing multiple a contention on some sysfs file
> because mlx driver tends to do some heavy lifting in its speed callback
> so it would be great to be able to interact with waiters during that
> time.

Haven't thought much about this, but it should be possible to use
rtnl_lock_killable. I think this should be a patch on its own with its
own justification though (to help bisecting). But that is easy to do
once the trylock logic is gone.

Thanks,
Antoine
Michal Hocko Oct. 6, 2021, 8:55 a.m. UTC | #3 
On Wed 06-10-21 10:03:41, Antoine Tenart wrote:
> Quoting Michal Hocko (2021-10-06 08:45:58)
> > On Tue 28-09-21 14:55:00, Antoine Tenart wrote:
> > > The ABBA deadlock avoided by using rtnl_trylock and restart_syscall was
> > > fixed in previous commits, we can now remove the use of this
> > > trylock/restart logic and have net-sysfs operations not spinning when
> > > rtnl is already taken.
> > 
> > Shouldn't those lock be interruptible or killable at least? As mentioned
> > in other reply we are seeing multiple a contention on some sysfs file
> > because mlx driver tends to do some heavy lifting in its speed callback
> > so it would be great to be able to interact with waiters during that
> > time.
> 
> Haven't thought much about this, but it should be possible to use
> rtnl_lock_killable. I think this should be a patch on its own with its
> own justification though (to help bisecting). But that is easy to do
> once the trylock logic is gone.

Agreed
diff mbox series

Patch

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index e754f00c117b..987b32fd8604 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -90,9 +90,7 @@  static ssize_t netdev_store(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		goto err;
 
-	if (!rtnl_trylock())
-		return restart_syscall();
-
+	rtnl_lock();
 	if (dev_isalive(netdev)) {
 		ret = (*set)(netdev, new);
 		if (ret == 0)
@@ -196,15 +194,7 @@  static ssize_t speed_show(struct device *dev,
 	struct net_device *netdev = to_net_dev(dev);
 	int ret = -EINVAL;
 
-	/* The check is also done in __ethtool_get_link_ksettings; this helps
-	 * returning early without hitting the trylock/restart below.
-	 */
-	if (!netdev->ethtool_ops->get_link_ksettings)
-		return -EOPNOTSUPP;
-
-	if (!rtnl_trylock())
-		return restart_syscall();
-
+	rtnl_lock();
 	if (netif_running(netdev)) {
 		struct ethtool_link_ksettings cmd;
 
@@ -222,15 +212,7 @@  static ssize_t duplex_show(struct device *dev,
 	struct net_device *netdev = to_net_dev(dev);
 	int ret = -EINVAL;
 
-	/* The check is also done in __ethtool_get_link_ksettings; this helps
-	 * returning early without hitting the trylock/restart below.
-	 */
-	if (!netdev->ethtool_ops->get_link_ksettings)
-		return -EOPNOTSUPP;
-
-	if (!rtnl_trylock())
-		return restart_syscall();
-
+	rtnl_lock();
 	if (netif_running(netdev)) {
 		struct ethtool_link_ksettings cmd;
 
@@ -427,9 +409,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();
-
+	rtnl_lock();
 	if (dev_isalive(netdev)) {
 		ret = dev_set_alias(netdev, buf, count);
 		if (ret < 0)
@@ -490,15 +470,7 @@  static ssize_t phys_port_id_show(struct device *dev,
 	struct net_device *netdev = to_net_dev(dev);
 	ssize_t ret = -EINVAL;
 
-	/* The check is also done in dev_get_phys_port_id; this helps returning
-	 * early without hitting the trylock/restart below.
-	 */
-	if (!netdev->netdev_ops->ndo_get_phys_port_id)
-		return -EOPNOTSUPP;
-
-	if (!rtnl_trylock())
-		return restart_syscall();
-
+	rtnl_lock();
 	if (dev_isalive(netdev)) {
 		struct netdev_phys_item_id ppid;
 
@@ -518,16 +490,7 @@  static ssize_t phys_port_name_show(struct device *dev,
 	struct net_device *netdev = to_net_dev(dev);
 	ssize_t ret = -EINVAL;
 
-	/* The checks are also done in dev_get_phys_port_name; this helps
-	 * returning early without hitting the trylock/restart below.
-	 */
-	if (!netdev->netdev_ops->ndo_get_phys_port_name &&
-	    !netdev->netdev_ops->ndo_get_devlink_port)
-		return -EOPNOTSUPP;
-
-	if (!rtnl_trylock())
-		return restart_syscall();
-
+	rtnl_lock();
 	if (dev_isalive(netdev)) {
 		char name[IFNAMSIZ];
 
@@ -547,16 +510,7 @@  static ssize_t phys_switch_id_show(struct device *dev,
 	struct net_device *netdev = to_net_dev(dev);
 	ssize_t ret = -EINVAL;
 
-	/* The checks are also done in dev_get_phys_port_name; this helps
-	 * returning early without hitting the trylock/restart below.
-	 */
-	if (!netdev->netdev_ops->ndo_get_port_parent_id &&
-	    !netdev->netdev_ops->ndo_get_devlink_port)
-		return -EOPNOTSUPP;
-
-	if (!rtnl_trylock())
-		return restart_syscall();
-
+	rtnl_lock();
 	if (dev_isalive(netdev)) {
 		struct netdev_phys_item_id ppid = { };
 
@@ -576,9 +530,7 @@  static ssize_t threaded_show(struct device *dev,
 	struct net_device *netdev = to_net_dev(dev);
 	ssize_t ret = -EINVAL;
 
-	if (!rtnl_trylock())
-		return restart_syscall();
-
+	rtnl_lock();
 	if (dev_isalive(netdev))
 		ret = sprintf(buf, fmt_dec, netdev->threaded);
 
@@ -1214,9 +1166,7 @@  static ssize_t traffic_class_show(struct netdev_queue *queue,
 	if (!netif_is_multiqueue(dev))
 		return -ENOENT;
 
-	if (!rtnl_trylock())
-		return restart_syscall();
-
+	rtnl_lock();
 	index = get_netdev_queue_index(queue);
 
 	/* If queue belongs to subordinate dev use its TC mapping */
@@ -1258,18 +1208,11 @@  static ssize_t tx_maxrate_store(struct netdev_queue *queue,
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	/* The check is also done later; this helps returning early without
-	 * hitting the trylock/restart below.
-	 */
-	if (!dev->netdev_ops->ndo_set_tx_maxrate)
-		return -EOPNOTSUPP;
-
 	err = kstrtou32(buf, 10, &rate);
 	if (err < 0)
 		return err;
 
-	if (!rtnl_trylock())
-		return restart_syscall();
+	rtnl_lock();
 
 	err = -EOPNOTSUPP;
 	if (dev->netdev_ops->ndo_set_tx_maxrate)
@@ -1460,8 +1403,7 @@  static ssize_t xps_cpus_show(struct netdev_queue *queue, char *buf)
 
 	index = get_netdev_queue_index(queue);
 
-	if (!rtnl_trylock())
-		return restart_syscall();
+	rtnl_lock();
 
 	/* If queue belongs to subordinate dev use its map */
 	dev = netdev_get_tx_queue(dev, index)->sb_dev ? : dev;
@@ -1507,11 +1449,7 @@  static ssize_t xps_cpus_store(struct netdev_queue *queue,
 		return err;
 	}
 
-	if (!rtnl_trylock()) {
-		free_cpumask_var(mask);
-		return restart_syscall();
-	}
-
+	rtnl_lock();
 	err = netif_set_xps_queue(dev, mask, index);
 	rtnl_unlock();
 
@@ -1531,9 +1469,7 @@  static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf)
 
 	index = get_netdev_queue_index(queue);
 
-	if (!rtnl_trylock())
-		return restart_syscall();
-
+	rtnl_lock();
 	tc = netdev_txq_to_tc(dev, index);
 	rtnl_unlock();
 	if (tc < 0)
@@ -1566,10 +1502,7 @@  static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf,
 		return err;
 	}
 
-	if (!rtnl_trylock()) {
-		bitmap_free(mask);
-		return restart_syscall();
-	}
+	rtnl_lock();
 
 	cpus_read_lock();
 	err = __netif_set_xps_queue(dev, mask, index, XPS_RXQS);