Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
Hello, Lai. On Wed, May 13, 2015 at 09:43:19AM +0800, Lai Jiangshan wrote: > >> I think the workqueue.c has too much complicated and rarely used APIs > >> and exposes too much in this way. No one can set the nice value > >> and the cpuallowed of a task atomically. > > > > What do you mean no one can? > > normal/general task. not kworker. > > no one can set the nice value and the cpumallowed of a normal task atomically. > > The kernel doesn't have such APIs: > > lock_and_get_task_cpus_allowed(task); > /* modify cpumask */ > set_cpus_allowed_ptr_and_unlock(); I'm still not following. What are you trying to say? > > So, we're now requiring workqueue users to take care of > > synchronization, disabling and reinstating WQ_SYSFS (what if userland > > hits those knobs at the same time?) > > I think there is no userland knobs when !WQ_SYSFS. So, fail apply attrs calls if the workqueue is exposed to userland? Are you serious? > > and poking into workqueue struct to determine the current values of the > > I think the copy version of cpumask, nice, numa values are same as > the workqueue struct have. No poking is required. > (Its own lock-protect-region is the ONLY entry to call > apply_workqueue_attrs()). And how would the caller know the current values? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
Hello, Lai. On Wed, May 13, 2015 at 09:43:19AM +0800, Lai Jiangshan wrote: I think the workqueue.c has too much complicated and rarely used APIs and exposes too much in this way. No one can set the nice value and the cpuallowed of a task atomically. What do you mean no one can? normal/general task. not kworker. no one can set the nice value and the cpumallowed of a normal task atomically. The kernel doesn't have such APIs: lock_and_get_task_cpus_allowed(task); /* modify cpumask */ set_cpus_allowed_ptr_and_unlock(); I'm still not following. What are you trying to say? So, we're now requiring workqueue users to take care of synchronization, disabling and reinstating WQ_SYSFS (what if userland hits those knobs at the same time?) I think there is no userland knobs when !WQ_SYSFS. So, fail apply attrs calls if the workqueue is exposed to userland? Are you serious? and poking into workqueue struct to determine the current values of the I think the copy version of cpumask, nice, numa values are same as the workqueue struct have. No poking is required. (Its own lock-protect-region is the ONLY entry to call apply_workqueue_attrs()). And how would the caller know the current values? Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
On 05/12/2015 09:22 PM, Tejun Heo wrote: > Hello, Lai. > > On Tue, May 12, 2015 at 10:15:28AM +0800, Lai Jiangshan wrote: >>> I'm not sure about this. Yeah, sure, it's a bit more lines of code >>> but at the same time this'd allow us to make the public interface >>> atomic too. What we prolly should do is changing the interface so >>> that we do >>> >>> attrs = prepare_workqueue_attrs(gfp_mask); /* allocate, lock & >>> copy */ >>> /* modify attrs as desired */ >>> commit_workqueue_attrs(attrs); /* apply, unlock and >>> free */ >> >> I think the workqueue.c has too much complicated and rarely used APIs >> and exposes too much in this way. No one can set the nice value >> and the cpuallowed of a task atomically. > > What do you mean no one can? normal/general task. not kworker. no one can set the nice value and the cpumallowed of a normal task atomically. The kernel doesn't have such APIs: lock_and_get_task_cpus_allowed(task); /* modify cpumask */ set_cpus_allowed_ptr_and_unlock(); > >> If the user want atomic-able, Her/he can just disable WQ_SYSFS >> on its workqueue and maintain a copy of the cpumask, nice, numa values >> under its own lock. > > So, we're now requiring workqueue users to take care of > synchronization, disabling and reinstating WQ_SYSFS (what if userland > hits those knobs at the same time?) I think there is no userland knobs when !WQ_SYSFS. > and poking into workqueue struct to determine the current values of the I think the copy version of cpumask, nice, numa values are same as the workqueue struct have. No poking is required. (Its own lock-protect-region is the ONLY entry to call apply_workqueue_attrs()). > attributes that the user is not > intereted in changing? This is a horrible interface. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
Hello, Lai. On Tue, May 12, 2015 at 10:15:28AM +0800, Lai Jiangshan wrote: > > I'm not sure about this. Yeah, sure, it's a bit more lines of code > > but at the same time this'd allow us to make the public interface > > atomic too. What we prolly should do is changing the interface so > > that we do > > > > attrs = prepare_workqueue_attrs(gfp_mask); /* allocate, lock & > > copy */ > > /* modify attrs as desired */ > > commit_workqueue_attrs(attrs); /* apply, unlock and > > free */ > > I think the workqueue.c has too much complicated and rarely used APIs > and exposes too much in this way. No one can set the nice value > and the cpuallowed of a task atomically. What do you mean no one can? > If the user want atomic-able, Her/he can just disable WQ_SYSFS > on its workqueue and maintain a copy of the cpumask, nice, numa values > under its own lock. So, we're now requiring workqueue users to take care of synchronization, disabling and reinstating WQ_SYSFS (what if userland hits those knobs at the same time?) and poking into workqueue struct to determine the current values of the attributes that the user is not intereted in changing? This is a horrible interface. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
On 05/12/2015 09:22 PM, Tejun Heo wrote: Hello, Lai. On Tue, May 12, 2015 at 10:15:28AM +0800, Lai Jiangshan wrote: I'm not sure about this. Yeah, sure, it's a bit more lines of code but at the same time this'd allow us to make the public interface atomic too. What we prolly should do is changing the interface so that we do attrs = prepare_workqueue_attrs(gfp_mask); /* allocate, lock copy */ /* modify attrs as desired */ commit_workqueue_attrs(attrs); /* apply, unlock and free */ I think the workqueue.c has too much complicated and rarely used APIs and exposes too much in this way. No one can set the nice value and the cpuallowed of a task atomically. What do you mean no one can? normal/general task. not kworker. no one can set the nice value and the cpumallowed of a normal task atomically. The kernel doesn't have such APIs: lock_and_get_task_cpus_allowed(task); /* modify cpumask */ set_cpus_allowed_ptr_and_unlock(); If the user want atomic-able, Her/he can just disable WQ_SYSFS on its workqueue and maintain a copy of the cpumask, nice, numa values under its own lock. So, we're now requiring workqueue users to take care of synchronization, disabling and reinstating WQ_SYSFS (what if userland hits those knobs at the same time?) I think there is no userland knobs when !WQ_SYSFS. and poking into workqueue struct to determine the current values of the I think the copy version of cpumask, nice, numa values are same as the workqueue struct have. No poking is required. (Its own lock-protect-region is the ONLY entry to call apply_workqueue_attrs()). attributes that the user is not intereted in changing? This is a horrible interface. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
Hello, Lai. On Tue, May 12, 2015 at 10:15:28AM +0800, Lai Jiangshan wrote: I'm not sure about this. Yeah, sure, it's a bit more lines of code but at the same time this'd allow us to make the public interface atomic too. What we prolly should do is changing the interface so that we do attrs = prepare_workqueue_attrs(gfp_mask); /* allocate, lock copy */ /* modify attrs as desired */ commit_workqueue_attrs(attrs); /* apply, unlock and free */ I think the workqueue.c has too much complicated and rarely used APIs and exposes too much in this way. No one can set the nice value and the cpuallowed of a task atomically. What do you mean no one can? If the user want atomic-able, Her/he can just disable WQ_SYSFS on its workqueue and maintain a copy of the cpumask, nice, numa values under its own lock. So, we're now requiring workqueue users to take care of synchronization, disabling and reinstating WQ_SYSFS (what if userland hits those knobs at the same time?) and poking into workqueue struct to determine the current values of the attributes that the user is not intereted in changing? This is a horrible interface. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
On 05/11/2015 10:59 PM, Tejun Heo wrote: > On Mon, May 11, 2015 at 05:35:51PM +0800, Lai Jiangshan wrote: >> workqueue_attrs is an internal-like structure and is exposed with >> apply_workqueue_attrs() whose user has to investigate the structure >> before use. >> >> And the apply_workqueue_attrs() API is inconvenient with the structure. >> The user (although there is no user yet currently) has to assemble >> several LoC to use: >> attrs = alloc_workqueue_attrs(); >> if (!attrs) >> return; >> attrs->nice = ...; >> copy cpumask; >> attrs->no_numa = ...; >> apply_workqueue_attrs(); >> free_workqueue_attrs(); >> >> It is too elaborate. This patch changes apply_workqueue_attrs() API, >> and one-line-code is enough to be called from user: >> apply_workqueue_attrs(wq, cpumask, nice, numa); >> >> This patch also reduces the code of workqueue.c, about -50 lines. >> wq_sysfs_prep_attrs() is removed, wq_[nice|cpumask|numa]_store() >> directly access to the ->unbound_attrs with the protection >> of apply_wqattrs_lock(); >> >> This patch is also a preparation patch of next patch which >> remove no_numa out from the structure workqueue_attrs which >> requires apply_workqueue_attrs() has an argument to pass numa affinity. > > I'm not sure about this. Yeah, sure, it's a bit more lines of code > but at the same time this'd allow us to make the public interface > atomic too. What we prolly should do is changing the interface so > that we do > > attrs = prepare_workqueue_attrs(gfp_mask); /* allocate, lock & > copy */ > /* modify attrs as desired */ > commit_workqueue_attrs(attrs); /* apply, unlock and > free */ I think the workqueue.c has too much complicated and rarely used APIs and exposes too much in this way. No one can set the nice value and the cpuallowed of a task atomically. If the user want atomic-able, Her/he can just disable WQ_SYSFS on its workqueue and maintain a copy of the cpumask, nice, numa values under its own lock. > > Thanks. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
On Mon, May 11, 2015 at 05:35:51PM +0800, Lai Jiangshan wrote: > workqueue_attrs is an internal-like structure and is exposed with > apply_workqueue_attrs() whose user has to investigate the structure > before use. > > And the apply_workqueue_attrs() API is inconvenient with the structure. > The user (although there is no user yet currently) has to assemble > several LoC to use: > attrs = alloc_workqueue_attrs(); > if (!attrs) > return; > attrs->nice = ...; > copy cpumask; > attrs->no_numa = ...; > apply_workqueue_attrs(); > free_workqueue_attrs(); > > It is too elaborate. This patch changes apply_workqueue_attrs() API, > and one-line-code is enough to be called from user: > apply_workqueue_attrs(wq, cpumask, nice, numa); > > This patch also reduces the code of workqueue.c, about -50 lines. > wq_sysfs_prep_attrs() is removed, wq_[nice|cpumask|numa]_store() > directly access to the ->unbound_attrs with the protection > of apply_wqattrs_lock(); > > This patch is also a preparation patch of next patch which > remove no_numa out from the structure workqueue_attrs which > requires apply_workqueue_attrs() has an argument to pass numa affinity. I'm not sure about this. Yeah, sure, it's a bit more lines of code but at the same time this'd allow us to make the public interface atomic too. What we prolly should do is changing the interface so that we do attrs = prepare_workqueue_attrs(gfp_mask); /* allocate, lock & copy */ /* modify attrs as desired */ commit_workqueue_attrs(attrs); /* apply, unlock and free */ Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/5] workqueue: don't expose workqueue_attrs to users
workqueue_attrs is an internal-like structure and is exposed with
apply_workqueue_attrs() whose user has to investigate the structure
before use.
And the apply_workqueue_attrs() API is inconvenient with the structure.
The user (although there is no user yet currently) has to assemble
several LoC to use:
attrs = alloc_workqueue_attrs();
if (!attrs)
return;
attrs->nice = ...;
copy cpumask;
attrs->no_numa = ...;
apply_workqueue_attrs();
free_workqueue_attrs();
It is too elaborate. This patch changes apply_workqueue_attrs() API,
and one-line-code is enough to be called from user:
apply_workqueue_attrs(wq, cpumask, nice, numa);
This patch also reduces the code of workqueue.c, about -50 lines.
wq_sysfs_prep_attrs() is removed, wq_[nice|cpumask|numa]_store()
directly access to the ->unbound_attrs with the protection
of apply_wqattrs_lock();
This patch is also a preparation patch of next patch which
remove no_numa out from the structure workqueue_attrs which
requires apply_workqueue_attrs() has an argument to pass numa affinity.
Signed-off-by: Lai Jiangshan
---
include/linux/workqueue.h | 18 +
kernel/workqueue.c| 173 ++
2 files changed, 70 insertions(+), 121 deletions(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 4618dd6..32e7c4b 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -119,20 +119,6 @@ struct delayed_work {
int cpu;
};
-/*
- * A struct for workqueue attributes. This can be used to change
- * attributes of an unbound workqueue.
- *
- * Unlike other fields, ->no_numa isn't a property of a worker_pool. It
- * only modifies how apply_workqueue_attrs() select pools and thus doesn't
- * participate in pool hash calculations or equality comparisons.
- */
-struct workqueue_attrs {
- int nice; /* nice level */
- cpumask_var_t cpumask;/* allowed CPUs */
- boolno_numa;/* disable NUMA affinity */
-};
-
static inline struct delayed_work *to_delayed_work(struct work_struct *work)
{
return container_of(work, struct delayed_work, work);
@@ -420,10 +406,8 @@ __alloc_workqueue_key(const char *fmt, unsigned int flags,
int max_active,
extern void destroy_workqueue(struct workqueue_struct *wq);
-struct workqueue_attrs *alloc_workqueue_attrs(gfp_t gfp_mask);
-void free_workqueue_attrs(struct workqueue_attrs *attrs);
int apply_workqueue_attrs(struct workqueue_struct *wq,
- const struct workqueue_attrs *attrs);
+ const cpumask_var_t cpumask, int nice, bool numa);
int workqueue_set_unbound_cpumask(cpumask_var_t cpumask);
extern bool queue_work_on(int cpu, struct workqueue_struct *wq,
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index efd9a3a..b08df98 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -106,6 +106,20 @@ enum {
};
/*
+ * A struct for workqueue attributes. This can be used to change
+ * attributes of an unbound workqueue.
+ *
+ * Unlike other fields, ->no_numa isn't a property of a worker_pool. It
+ * only modifies how apply_workqueue_attrs() select pools and thus doesn't
+ * participate in pool hash calculations or equality comparisons.
+ */
+struct workqueue_attrs {
+ int nice; /* nice level */
+ cpumask_var_t cpumask;/* allowed CPUs */
+ boolno_numa;/* disable NUMA affinity */
+};
+
+/*
* Structure fields follow one of the following exclusion rules.
*
* I: Modifiable by initialization/destruction paths and read-only for
@@ -307,6 +321,8 @@ static bool workqueue_freezing; /* PL: have wqs
started freezing? */
static cpumask_var_t wq_unbound_cpumask; /* PL: low level cpumask for all
unbound wqs */
+static const int std_nice[NR_STD_WORKER_POOLS] = { 0, HIGHPRI_NICE_LEVEL };
+
/* the per-cpu worker pools */
static DEFINE_PER_CPU_SHARED_ALIGNED(struct worker_pool [NR_STD_WORKER_POOLS],
cpu_worker_pools);
@@ -316,12 +332,6 @@ static DEFINE_IDR(worker_pool_idr);/* PR: idr of
all pools */
/* PL: hash of all unbound pools keyed by pool->attrs */
static DEFINE_HASHTABLE(unbound_pool_hash, UNBOUND_POOL_HASH_ORDER);
-/* I: attributes used when instantiating standard unbound pools on demand */
-static struct workqueue_attrs *unbound_std_wq_attrs[NR_STD_WORKER_POOLS];
-
-/* I: attributes used when instantiating ordered pools on demand */
-static struct workqueue_attrs *ordered_wq_attrs[NR_STD_WORKER_POOLS];
-
struct workqueue_struct *system_wq __read_mostly;
EXPORT_SYMBOL(system_wq);
struct workqueue_struct *system_highpri_wq __read_mostly;
@@ -338,8 +348,6 @@ struct workqueue_struct
*system_freezable_power_efficient_wq __read_mostly;
[PATCH 4/5] workqueue: don't expose workqueue_attrs to users
workqueue_attrs is an internal-like structure and is exposed with
apply_workqueue_attrs() whose user has to investigate the structure
before use.
And the apply_workqueue_attrs() API is inconvenient with the structure.
The user (although there is no user yet currently) has to assemble
several LoC to use:
attrs = alloc_workqueue_attrs();
if (!attrs)
return;
attrs-nice = ...;
copy cpumask;
attrs-no_numa = ...;
apply_workqueue_attrs();
free_workqueue_attrs();
It is too elaborate. This patch changes apply_workqueue_attrs() API,
and one-line-code is enough to be called from user:
apply_workqueue_attrs(wq, cpumask, nice, numa);
This patch also reduces the code of workqueue.c, about -50 lines.
wq_sysfs_prep_attrs() is removed, wq_[nice|cpumask|numa]_store()
directly access to the -unbound_attrs with the protection
of apply_wqattrs_lock();
This patch is also a preparation patch of next patch which
remove no_numa out from the structure workqueue_attrs which
requires apply_workqueue_attrs() has an argument to pass numa affinity.
Signed-off-by: Lai Jiangshan la...@cn.fujitsu.com
---
include/linux/workqueue.h | 18 +
kernel/workqueue.c| 173 ++
2 files changed, 70 insertions(+), 121 deletions(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 4618dd6..32e7c4b 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -119,20 +119,6 @@ struct delayed_work {
int cpu;
};
-/*
- * A struct for workqueue attributes. This can be used to change
- * attributes of an unbound workqueue.
- *
- * Unlike other fields, -no_numa isn't a property of a worker_pool. It
- * only modifies how apply_workqueue_attrs() select pools and thus doesn't
- * participate in pool hash calculations or equality comparisons.
- */
-struct workqueue_attrs {
- int nice; /* nice level */
- cpumask_var_t cpumask;/* allowed CPUs */
- boolno_numa;/* disable NUMA affinity */
-};
-
static inline struct delayed_work *to_delayed_work(struct work_struct *work)
{
return container_of(work, struct delayed_work, work);
@@ -420,10 +406,8 @@ __alloc_workqueue_key(const char *fmt, unsigned int flags,
int max_active,
extern void destroy_workqueue(struct workqueue_struct *wq);
-struct workqueue_attrs *alloc_workqueue_attrs(gfp_t gfp_mask);
-void free_workqueue_attrs(struct workqueue_attrs *attrs);
int apply_workqueue_attrs(struct workqueue_struct *wq,
- const struct workqueue_attrs *attrs);
+ const cpumask_var_t cpumask, int nice, bool numa);
int workqueue_set_unbound_cpumask(cpumask_var_t cpumask);
extern bool queue_work_on(int cpu, struct workqueue_struct *wq,
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index efd9a3a..b08df98 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -106,6 +106,20 @@ enum {
};
/*
+ * A struct for workqueue attributes. This can be used to change
+ * attributes of an unbound workqueue.
+ *
+ * Unlike other fields, -no_numa isn't a property of a worker_pool. It
+ * only modifies how apply_workqueue_attrs() select pools and thus doesn't
+ * participate in pool hash calculations or equality comparisons.
+ */
+struct workqueue_attrs {
+ int nice; /* nice level */
+ cpumask_var_t cpumask;/* allowed CPUs */
+ boolno_numa;/* disable NUMA affinity */
+};
+
+/*
* Structure fields follow one of the following exclusion rules.
*
* I: Modifiable by initialization/destruction paths and read-only for
@@ -307,6 +321,8 @@ static bool workqueue_freezing; /* PL: have wqs
started freezing? */
static cpumask_var_t wq_unbound_cpumask; /* PL: low level cpumask for all
unbound wqs */
+static const int std_nice[NR_STD_WORKER_POOLS] = { 0, HIGHPRI_NICE_LEVEL };
+
/* the per-cpu worker pools */
static DEFINE_PER_CPU_SHARED_ALIGNED(struct worker_pool [NR_STD_WORKER_POOLS],
cpu_worker_pools);
@@ -316,12 +332,6 @@ static DEFINE_IDR(worker_pool_idr);/* PR: idr of
all pools */
/* PL: hash of all unbound pools keyed by pool-attrs */
static DEFINE_HASHTABLE(unbound_pool_hash, UNBOUND_POOL_HASH_ORDER);
-/* I: attributes used when instantiating standard unbound pools on demand */
-static struct workqueue_attrs *unbound_std_wq_attrs[NR_STD_WORKER_POOLS];
-
-/* I: attributes used when instantiating ordered pools on demand */
-static struct workqueue_attrs *ordered_wq_attrs[NR_STD_WORKER_POOLS];
-
struct workqueue_struct *system_wq __read_mostly;
EXPORT_SYMBOL(system_wq);
struct workqueue_struct *system_highpri_wq __read_mostly;
@@ -338,8 +348,6 @@ struct workqueue_struct
*system_freezable_power_efficient_wq __read_mostly;
Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
On Mon, May 11, 2015 at 05:35:51PM +0800, Lai Jiangshan wrote: workqueue_attrs is an internal-like structure and is exposed with apply_workqueue_attrs() whose user has to investigate the structure before use. And the apply_workqueue_attrs() API is inconvenient with the structure. The user (although there is no user yet currently) has to assemble several LoC to use: attrs = alloc_workqueue_attrs(); if (!attrs) return; attrs-nice = ...; copy cpumask; attrs-no_numa = ...; apply_workqueue_attrs(); free_workqueue_attrs(); It is too elaborate. This patch changes apply_workqueue_attrs() API, and one-line-code is enough to be called from user: apply_workqueue_attrs(wq, cpumask, nice, numa); This patch also reduces the code of workqueue.c, about -50 lines. wq_sysfs_prep_attrs() is removed, wq_[nice|cpumask|numa]_store() directly access to the -unbound_attrs with the protection of apply_wqattrs_lock(); This patch is also a preparation patch of next patch which remove no_numa out from the structure workqueue_attrs which requires apply_workqueue_attrs() has an argument to pass numa affinity. I'm not sure about this. Yeah, sure, it's a bit more lines of code but at the same time this'd allow us to make the public interface atomic too. What we prolly should do is changing the interface so that we do attrs = prepare_workqueue_attrs(gfp_mask); /* allocate, lock copy */ /* modify attrs as desired */ commit_workqueue_attrs(attrs); /* apply, unlock and free */ Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users
On 05/11/2015 10:59 PM, Tejun Heo wrote: On Mon, May 11, 2015 at 05:35:51PM +0800, Lai Jiangshan wrote: workqueue_attrs is an internal-like structure and is exposed with apply_workqueue_attrs() whose user has to investigate the structure before use. And the apply_workqueue_attrs() API is inconvenient with the structure. The user (although there is no user yet currently) has to assemble several LoC to use: attrs = alloc_workqueue_attrs(); if (!attrs) return; attrs-nice = ...; copy cpumask; attrs-no_numa = ...; apply_workqueue_attrs(); free_workqueue_attrs(); It is too elaborate. This patch changes apply_workqueue_attrs() API, and one-line-code is enough to be called from user: apply_workqueue_attrs(wq, cpumask, nice, numa); This patch also reduces the code of workqueue.c, about -50 lines. wq_sysfs_prep_attrs() is removed, wq_[nice|cpumask|numa]_store() directly access to the -unbound_attrs with the protection of apply_wqattrs_lock(); This patch is also a preparation patch of next patch which remove no_numa out from the structure workqueue_attrs which requires apply_workqueue_attrs() has an argument to pass numa affinity. I'm not sure about this. Yeah, sure, it's a bit more lines of code but at the same time this'd allow us to make the public interface atomic too. What we prolly should do is changing the interface so that we do attrs = prepare_workqueue_attrs(gfp_mask); /* allocate, lock copy */ /* modify attrs as desired */ commit_workqueue_attrs(attrs); /* apply, unlock and free */ I think the workqueue.c has too much complicated and rarely used APIs and exposes too much in this way. No one can set the nice value and the cpuallowed of a task atomically. If the user want atomic-able, Her/he can just disable WQ_SYSFS on its workqueue and maintain a copy of the cpumask, nice, numa values under its own lock. Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
