[PATCH wq/for-next 1/2] workqueue: let subsystem core create the cpumask attribute

[PATCH wq/for-next 1/2] workqueue: let subsystem core create the cpumask attribute

[Julian Wiedmann]

Wrap the cpumask attribute in an ATTRIBUTE_GROUPS() macro, so that
subsys_virtual_register() can add it to the dev_root device for us.

Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
 kernel/workqueue.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 104e3ef04e33..817dc2d7438a 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5625,15 +5625,15 @@ static struct device_attribute wq_sysfs_cpumask_attr =
 	__ATTR(cpumask, 0644, wq_unbound_cpumask_show,
 	       wq_unbound_cpumask_store);
 
+static struct attribute *wq_sysfs_dev_root_attrs[] = {
+	&wq_sysfs_cpumask_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wq_sysfs_dev_root);
+
 static int __init wq_sysfs_init(void)
 {
-	int err;
-
-	err = subsys_virtual_register(&wq_subsys, NULL);
-	if (err)
-		return err;
-
-	return device_create_file(wq_subsys.dev_root, &wq_sysfs_cpumask_attr);
+	return subsys_virtual_register(&wq_subsys, wq_sysfs_dev_root_groups);
 }
 core_initcall(wq_sysfs_init);
 
-- 
2.25.1

[PATCH wq/for-next 2/2] workqueue: let device core create the WQ_UNBOUND attributes

[Julian Wiedmann]

Wrap the attributes for a WQ_UNBOUND workqueue in ATTRIBUTE_GROUPS(),
and wire them up in dev->groups so that the device core can manage them
for us.

As device_add() will add such attributes _prior_ to raising the KOBJ_ADD
uevent, this also makes the initial uevent suppression unnecessary.

Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
---
 kernel/workqueue.c | 51 ++++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 29 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 817dc2d7438a..629859ac5262 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5449,6 +5449,9 @@ static ssize_t wq_pool_ids_show(struct device *dev,
 	return written;
 }
 
+static struct device_attribute wq_sysfs_unbound_attr_pool_ids =
+	__ATTR(pool_ids, 0444, wq_pool_ids_show, NULL);
+
 static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
@@ -5502,6 +5505,9 @@ static ssize_t wq_nice_store(struct device *dev, struct device_attribute *attr,
 	return ret ?: count;
 }
 
+static struct device_attribute wq_sysfs_unbound_attr_nice =
+	__ATTR(nice, 0644, wq_nice_show, wq_nice_store);
+
 static ssize_t wq_cpumask_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
@@ -5539,6 +5545,9 @@ static ssize_t wq_cpumask_store(struct device *dev,
 	return ret ?: count;
 }
 
+static struct device_attribute wq_sysfs_unbound_attr_cpumask =
+	__ATTR(cpumask, 0644, wq_cpumask_show, wq_cpumask_store);
+
 static ssize_t wq_numa_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
@@ -5578,13 +5587,17 @@ static ssize_t wq_numa_store(struct device *dev, struct device_attribute *attr,
 	return ret ?: count;
 }
 
-static struct device_attribute wq_sysfs_unbound_attrs[] = {
-	__ATTR(pool_ids, 0444, wq_pool_ids_show, NULL),
-	__ATTR(nice, 0644, wq_nice_show, wq_nice_store),
-	__ATTR(cpumask, 0644, wq_cpumask_show, wq_cpumask_store),
-	__ATTR(numa, 0644, wq_numa_show, wq_numa_store),
-	__ATTR_NULL,
+static struct device_attribute wq_sysfs_unbound_attr_numa =
+	__ATTR(numa, 0644, wq_numa_show, wq_numa_store);
+
+static struct attribute *wq_sysfs_unbound_attrs[] = {
+	&wq_sysfs_unbound_attr_pool_ids.attr,
+	&wq_sysfs_unbound_attr_nice.attr,
+	&wq_sysfs_unbound_attr_cpumask.attr,
+	&wq_sysfs_unbound_attr_numa.attr,
+	NULL,
 };
+ATTRIBUTE_GROUPS(wq_sysfs_unbound);
 
 static struct bus_type wq_subsys = {
 	.name				= "workqueue",
@@ -5679,37 +5692,17 @@ int workqueue_sysfs_register(struct workqueue_struct *wq)
 	wq_dev->wq = wq;
 	wq_dev->dev.bus = &wq_subsys;
 	wq_dev->dev.release = wq_device_release;
+	if (wq->flags & WQ_UNBOUND)
+		wq_dev->dev.groups = wq_sysfs_unbound_groups;
 	dev_set_name(&wq_dev->dev, "%s", wq->name);
 
-	/*
-	 * unbound_attrs are created separately.  Suppress uevent until
-	 * everything is ready.
-	 */
-	dev_set_uevent_suppress(&wq_dev->dev, true);
-
 	ret = device_register(&wq_dev->dev);
 	if (ret) {
 		put_device(&wq_dev->dev);
 		wq->wq_dev = NULL;
-		return ret;
-	}
-
-	if (wq->flags & WQ_UNBOUND) {
-		struct device_attribute *attr;
-
-		for (attr = wq_sysfs_unbound_attrs; attr->attr.name; attr++) {
-			ret = device_create_file(&wq_dev->dev, attr);
-			if (ret) {
-				device_unregister(&wq_dev->dev);
-				wq->wq_dev = NULL;
-				return ret;
-			}
-		}
 	}
 
-	dev_set_uevent_suppress(&wq_dev->dev, false);
-	kobject_uevent(&wq_dev->dev.kobj, KOBJ_ADD);
-	return 0;
+	return ret;
 }
 
 /**
-- 
2.25.1

Re: [PATCH wq/for-next 2/2] workqueue: let device core create the WQ_UNBOUND attributes

[Greg Kroah-Hartman]

On Mon, Jun 07, 2021 at 11:44:20AM +0200, Julian Wiedmann wrote:
> Wrap the attributes for a WQ_UNBOUND workqueue in ATTRIBUTE_GROUPS(),
> and wire them up in dev->groups so that the device core can manage them
> for us.
> 
> As device_add() will add such attributes _prior_ to raising the KOBJ_ADD
> uevent, this also makes the initial uevent suppression unnecessary.
> 
> Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
> ---
>  kernel/workqueue.c | 51 ++++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 817dc2d7438a..629859ac5262 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -5449,6 +5449,9 @@ static ssize_t wq_pool_ids_show(struct device *dev,
>  	return written;
>  }
>  
> +static struct device_attribute wq_sysfs_unbound_attr_pool_ids =
> +	__ATTR(pool_ids, 0444, wq_pool_ids_show, NULL);

__ATTR_RO() please.

> +
>  static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
>  			    char *buf)
>  {
> @@ -5502,6 +5505,9 @@ static ssize_t wq_nice_store(struct device *dev, struct device_attribute *attr,
>  	return ret ?: count;
>  }
>  
> +static struct device_attribute wq_sysfs_unbound_attr_nice =
> +	__ATTR(nice, 0644, wq_nice_show, wq_nice_store);

__ATTR_RW()

> +
>  static ssize_t wq_cpumask_show(struct device *dev,
>  			       struct device_attribute *attr, char *buf)
>  {
> @@ -5539,6 +5545,9 @@ static ssize_t wq_cpumask_store(struct device *dev,
>  	return ret ?: count;
>  }
>  
> +static struct device_attribute wq_sysfs_unbound_attr_cpumask =
> +	__ATTR(cpumask, 0644, wq_cpumask_show, wq_cpumask_store);

__ATTR_RW()

thanks,

greg k-h

Re: [PATCH wq/for-next 2/2] workqueue: let device core create the WQ_UNBOUND attributes

[Julian Wiedmann]

On 07.06.21 13:27, Greg Kroah-Hartman wrote:
> On Mon, Jun 07, 2021 at 11:44:20AM +0200, Julian Wiedmann wrote:
>> Wrap the attributes for a WQ_UNBOUND workqueue in ATTRIBUTE_GROUPS(),
>> and wire them up in dev->groups so that the device core can manage them
>> for us.
>>
>> As device_add() will add such attributes _prior_ to raising the KOBJ_ADD
>> uevent, this also makes the initial uevent suppression unnecessary.
>>
>> Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
>> ---
>>  kernel/workqueue.c | 51 ++++++++++++++++++++--------------------------
>>  1 file changed, 22 insertions(+), 29 deletions(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 817dc2d7438a..629859ac5262 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -5449,6 +5449,9 @@ static ssize_t wq_pool_ids_show(struct device *dev,
>>  	return written;
>>  }
>>  
>> +static struct device_attribute wq_sysfs_unbound_attr_pool_ids =
>> +	__ATTR(pool_ids, 0444, wq_pool_ids_show, NULL);
> 
> __ATTR_RO() please.
> 

I did consider using DEVICE_ATTR_*(), yes. But renaming all the _show and _store
functions added too much unrelated churn for my taste. So let's maybe do this
as a follow-on?

>> +
>>  static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
>>  			    char *buf)
>>  {
>> @@ -5502,6 +5505,9 @@ static ssize_t wq_nice_store(struct device *dev, struct device_attribute *attr,
>>  	return ret ?: count;
>>  }
>>  
>> +static struct device_attribute wq_sysfs_unbound_attr_nice =
>> +	__ATTR(nice, 0644, wq_nice_show, wq_nice_store);
> 
> __ATTR_RW()
> 
>> +
>>  static ssize_t wq_cpumask_show(struct device *dev,
>>  			       struct device_attribute *attr, char *buf)
>>  {
>> @@ -5539,6 +5545,9 @@ static ssize_t wq_cpumask_store(struct device *dev,
>>  	return ret ?: count;
>>  }
>>  
>> +static struct device_attribute wq_sysfs_unbound_attr_cpumask =
>> +	__ATTR(cpumask, 0644, wq_cpumask_show, wq_cpumask_store);
> 
> __ATTR_RW()
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH wq/for-next 2/2] workqueue: let device core create the WQ_UNBOUND attributes

[Greg Kroah-Hartman]

On Mon, Jun 07, 2021 at 01:34:49PM +0300, Julian Wiedmann wrote:
> On 07.06.21 13:27, Greg Kroah-Hartman wrote:
> > On Mon, Jun 07, 2021 at 11:44:20AM +0200, Julian Wiedmann wrote:
> >> Wrap the attributes for a WQ_UNBOUND workqueue in ATTRIBUTE_GROUPS(),
> >> and wire them up in dev->groups so that the device core can manage them
> >> for us.
> >>
> >> As device_add() will add such attributes _prior_ to raising the KOBJ_ADD
> >> uevent, this also makes the initial uevent suppression unnecessary.
> >>
> >> Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
> >> ---
> >>  kernel/workqueue.c | 51 ++++++++++++++++++++--------------------------
> >>  1 file changed, 22 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> >> index 817dc2d7438a..629859ac5262 100644
> >> --- a/kernel/workqueue.c
> >> +++ b/kernel/workqueue.c
> >> @@ -5449,6 +5449,9 @@ static ssize_t wq_pool_ids_show(struct device *dev,
> >>  	return written;
> >>  }
> >>  
> >> +static struct device_attribute wq_sysfs_unbound_attr_pool_ids =
> >> +	__ATTR(pool_ids, 0444, wq_pool_ids_show, NULL);
> > 
> > __ATTR_RO() please.
> > 
> 
> I did consider using DEVICE_ATTR_*(), yes. But renaming all the _show and _store
> functions added too much unrelated churn for my taste. So let's maybe do this
> as a follow-on?

Wait, oops, yes, do NOT use __ATTR() for a device attribute, use
DEVICE_ATTR_RO() please.

And yes, if you want to do it as a pater patch in this series that is
fine, just say you are moving the attribute definitions closer to the
callbacks in this patch.

thanks,

greg k-h