Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM
Hello, On Fri, Jan 26, 2018 at 09:08:02AM +0100, Johannes Berg wrote: > Not that you mention it ... Honestly though, wifi connections break all > the time, so not sure you'd really want that. But anyway, that's what > we have. I'd be a lot happier to remove WQ_MEM_RECLAIM from wireless drivers too, and glancing the code, it looked like there already are so many holes that whether having that flag or not shouldn't matter all that much. It was just difficult for me to make a case for removing it preemptively. If you're up for it, please be my guest. Thanks. -- tejun
Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM
Hello,
On Thu, Jan 25, 2018 at 10:01:26PM +0100, Johannes Berg wrote:
> I guess we should just ask Tejun :-)
>
> Tejun, the problem was a report that a WQ_MEM_RECLAIM workqueue is
> flushing another that isn't, and it turns out that lots of wireless
> drivers are using WQ_MEM_RECLAIM for some reason.
Yeah, that came up a couple years ago. IIRC, there wasn't a definite
answer but the sentiment seemed that things like nfs over wireless
should probably considered. No idea how serious that concern is.
> Arend said:
> > > > > Maybe a hint in the documentation, that a work item on a
> > > WQ_MEM_RECLAIM
> > > > > queue must not call flush of an !WQ_MEM_RECLAIM queue would be nice.
> > > > > Maybe it's kind of obvious, but there is also a reminder not to
> > > forget
> > > > > that flag, if a queue may have work items that reclaim memory
> > > >
> > > > Yeah, honestly, I'm not really sure either. Clearly we can't set it,
> > > > but other drivers also set it...
So, anything which can sit in memory reclaim path needs to have that
flag set and having that flag set automatically means that it can't
depend on anything which isn't protected the same way as that'd break
that protection.
> > > That triggered something in my memory. So indeed we use it in brcmfmac
> > > as well. We used create_singlethread_workqueue(), but I wanted to avoid
> > > snprintf and specify the name format so switched to using
> > > alloc_ordered_workqueue() keeping WQ_MEM_RECLAIM as per the macro
> > > definition.
> >
> > #define create_singlethread_workqueue(name) \
> > alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name)
> >
> > > Don't recall why I dropped the __WQ_LEGACY flag though.
The only thing that flag does is disabling the flush dependency check
which is necessary because in the old implementation, all workqueues
were basically WQ_MEM_RECLAIM workqueues leading to spurious
triggering of the warnings.
Thanks.
--
tejun
Re: [PATCH v2] cfg80211: Remove deprecated create_singlethread_workqueue
On Wed, Aug 31, 2016 at 12:35:07AM +0530, Bhaktipriya Shridhar wrote: > The workqueue "cfg80211_wq" is involved in cleanup, scan and event related > works. It queues multiple work items &rdev->event_work, > &rdev->dfs_update_channels_wk, > &wiphy_to_rdev(request->wiphy)->scan_done_wk, > &wiphy_to_rdev(wiphy)->sched_scan_results_wk, which require strict > execution ordering. > Hence, an ordered dedicated workqueue has been used. > > Since it's a wireless driver, WQ_MEM_RECLAIM has been set to ensure > forward progress under memory pressure. > > Signed-off-by: Bhaktipriya Shridhar Acked-by: Tejun Heo Thanks. -- tejun
Re: [PATCH] libertas_tf: Remove create_workqueue
On Wed, Jun 08, 2016 at 01:38:53AM +0530, Bhaktipriya Shridhar wrote: > alloc_workqueue replaces deprecated create_workqueue(). > > A dedicated workqueue has been used since the workitem (viz > &priv->cmd_work per priv, which maps to lbtf_cmd_work) is involved in > actual command processing and may be used on a memory reclaim path. > The workitems require forward progress under memory pressure and hence, > WQ_MEM_RECLAIM has been set. Since there are only a fixed number of work > items, explicit concurrency limit is unnecessary here. > > Signed-off-by: Bhaktipriya Shridhar Acked-by: Tejun Heo Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: wireless: marvell: libertas: Remove create_workqueue
On Sat, Jun 04, 2016 at 07:29:01PM +0530, Bhaktipriya Shridhar wrote: > alloc_workqueue replaces deprecated create_workqueue(). > > In if_sdio.c, the workqueue card->workqueue has workitem > &card->packet_worker, which is mapped to if_sdio_host_to_card_worker. > The workitem is involved in sending packets to firmware. > Forward progress under memory pressure is a requirement here. > > In if_spi.c, the workqueue card->workqueue has workitem > &card->packet_worker, which is mapped to if_spi_host_to_card_worker. > The workitem is involved in sending command packets from the host. > Forward progress under memory pressure is a requirement here. > > Dedicated workqueues have been used in both cases since the workitems > on the workqueues are involved in normal device operation with > WQ_MEM_RECLAIM set to gurantee forward progress under memory pressure. > Since there are only a fixed number of work items, explicit concurrency > limit is unnecessary. > > flush_workqueue is unnecessary since destroy_workqueue() itself calls > drain_workqueue() which flushes repeatedly till the workqueue > becomes empty. Hence the calls to flush_workqueue() before > destroy_workqueue() have been dropped. > > Signed-off-by: Bhaktipriya Shridhar Acked-by: Tejun Heo Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFD] workqueue: WQ_MEM_RECLAIM usage in network drivers
Hello, On Fri, Mar 18, 2016 at 05:24:05PM -0400, J. Bruce Fields wrote: > > But does that actually work? It's pointless to add WQ_MEM_RECLAIM to > > workqueues unless all other things are also guaranteed to make forward > > progress regardless of memory pressure. > > It's supposed to work. > > Also note there was a bunch of work done to allow swap on NFS: see > a564b8f0 "nfs: enable swap on NFS". Alright, WQ_MEM_RECLAIM for ethernet devices, then. > I use NFS mounts over wifi at home. I may just be odd. I seem to > recall some bug reports about suspend vs. NFS--were those also on > laptops using NFS? > > I wonder if home media centers might do writes over wifi to network > storage? > > Googling for "nfs wifi" turns up lots of individuals doing this. > > My first impulse is to say that it's probably not perfect but that we > shouldn't make it worse. If everything else is working, I'd be happy to throw in WQ_MEM_RECLAIM but I really don't want to add it if it doesn't actually achieve the goal. Can a wireless person chime in here? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFD] workqueue: WQ_MEM_RECLAIM usage in network drivers
Hello, Jeff. On Thu, Mar 17, 2016 at 09:32:16PM -0400, Jeff Layton wrote: > > * Are network devices expected to be able to serve as a part of > > storage stack which is depended upon for memory reclamation? > > I think they should be. Cached NFS pages can consume a lot of memory, > and flushing them generally takes network device access. But does that actually work? It's pointless to add WQ_MEM_RECLAIM to workqueues unless all other things are also guaranteed to make forward progress regardless of memory pressure. > > * If so, are all the pieces in place for that to work for all (or at > > least most) network devices? If it's only for a subset of NICs, how > > can one tell whether a given driver needs forward progress guarantee > > or not? > > > > * I assume that wireless drivers aren't and can't be used in this > > fashion. Is that a correction assumption? > > > > People do mount NFS over wireless interfaces. It's not terribly common > though, in my experience. Ditto, I'm very skeptical that this actually works in practice and people expect and depend on it. I don't follow wireless development closely but haven't heard anyone talking about reserving memory pools or people complaining about wireless being the cause of OOM. So, I really want to avoid spraying WQ_MEM_RECLAIM if it doesn't serve actual purposes. It's wasteful, sets bad precedences and confuses future readers. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] iwlwifi: dvm: convert create_singlethread_workqueue() to alloc_workqueue()
Hello,
On Thu, Mar 17, 2016 at 01:43:22PM +0100, Johannes Berg wrote:
> On Thu, 2016-03-17 at 20:37 +0800, Eva Rachel Retuya wrote:
> > Use alloc_workqueue() to allocate the workqueue instead of
> > create_singlethread_workqueue() since the latter is deprecated and is
> > scheduled for removal.
>
> Scheduled where?
They've been deprecated for years now. I should note that in the
header.
> > static void iwl_setup_deferred_work(struct iwl_priv *priv)
> > {
> > - priv->workqueue = create_singlethread_workqueue(DRV_NAME);
> > + priv->workqueue = alloc_workqueue(DRV_NAME, WQ_HIGHPRI |
> > WQ_UNBOUND |
> > + WQ_MEM_RECLAIM, 1);
>
> Seems like you should use alloc_ordered_workqueue() though? That also
> gets you UNBOUND immediately, and the "1".
Right, this one should have been alloc_ordered_workqueue().
> I'm not really sure HIGHPRI is needed either.
So, no WQ_MEM_RECLAIM either then, I suppose? What are the latency
requirements here - what happens if a thermal management work gets
delayed?
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFD] workqueue: WQ_MEM_RECLAIM usage in network drivers
Hello, Years ago, workqueue got reimplemented to use common worker pools across different workqueues and a new set of more expressive workqueue creation APIs, alloc_*workqueue() were introduced. The old create_*workqueue() became simple wrappers around alloc_*workqueue() with the most conservative parameters. The plan has always been to examine each usage and convert to the new interface with parameters actually required for the use case. One important flag to decide upon is WQ_MEM_RECLAIM, which declares that the workqueue may be depended upon during memory reclaim and thus must be able to make forward-progress even when further memory can't be allocated without reclaiming some. Of the network drivers which already use alloc_*workqueue() interface, some specify this flag and I'm wondering what the guidelines should be here. * Are network devices expected to be able to serve as a part of storage stack which is depended upon for memory reclamation? * If so, are all the pieces in place for that to work for all (or at least most) network devices? If it's only for a subset of NICs, how can one tell whether a given driver needs forward progress guarantee or not? * I assume that wireless drivers aren't and can't be used in this fashion. Is that a correction assumption? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Outreachy kernel] [PATCH v2] iwlwifi: dvm: use alloc_ordered_workqueue()
On Fri, Mar 18, 2016 at 12:19:21AM +0800, Eva Rachel Retuya wrote: > Use alloc_ordered_workqueue() to allocate the workqueue instead of > create_singlethread_workqueue() since the latter is deprecated and is > scheduled > for removal. > > There are work items doing related operations that shouldn't be swapped when > queued in a certain order hence preserve the strict execution ordering of a > single threaded (ST) workqueue by switching to alloc_ordered_workqueue(). > > WQ_MEM_RECLAIM flag is not needed since the worker is not supposed to free > memory. I think "not depended during memory reclaim" probalby is a better way to describe it. > Signed-off-by: Eva Rachel Retuya But other than that, Acked-by: Tejun Heo Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/8] kernel/params.c: generalize bool_enable_only
Hello,
On Wed, Apr 22, 2015 at 02:55:06PM -0700, Luis R. Rodriguez wrote:
> +int param_set_bool_enable_only(const char *val, const struct kernel_param
> *kp)
> +{
> + int err = 0;
> + bool new_value;
> + bool orig_value = *(bool *)kp->arg;
> + struct kernel_param dummy_kp = *kp;
> +
> + dummy_kp.arg = &new_value;
> +
> + err = param_set_bool(val, &dummy_kp);
> + if (err)
> + return err;
> +
> + /* Don't let them unset it once it's set! */
> + if (!new_value && orig_value)
> + return -EROFS;
I know that this was moved from another place but as we're making it
generic now I'm a bit curious about -EROFS. Wouldn't -EINVAL be a
more conventional choice here?
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 4/6] moduleparam.h: add module_param_config_*() helpers
Hello, Luis. On Tue, Apr 21, 2015 at 06:55:16PM +0200, Luis R. Rodriguez wrote: > A use then would be for instance: > > module_param_config_on_off(power_efficient, wq_power_efficient, 0444, > IS_ENABLED(CONFIG_WQ_POWER_EFFICIENT_DEFAULT)); > > this as an alternative would enable use of other static / global variables but > I'm not sure if these are good use cases to promote, given that all this is to > help with initial set up, so I believe the restrictions are for the better. I was thinking more of cases where CONFIG should be inverted or and/or'd. In general I don't think we conventionally embed IS_ENABLED() in this sort of macros. It just jumps at me as a weird restriction. What do others think? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 4/6] moduleparam.h: add module_param_config_*() helpers
On Mon, Apr 20, 2015 at 04:30:35PM -0700, Luis R. Rodriguez wrote: > /** > + * module_param_config_on_off - bool parameter with run time override > + * @name: a valid C identifier which is the parameter name. > + * @value: the actual lvalue to alter. > + * @perm: visibility in sysfs. > + * @config: kernel parameter which will enable this option if this > + * kernel configuration option has been enabled. > + * > + * This lets you define a bool module paramter which by default will be > + * set to true if the config option has been set on your kernel's > + * configuration, otherwise it is set to false. > + */ > +#define module_param_config_on_off(name, var, perm, config) \ > + static bool var = IS_ENABLED(config); \ > + module_param_named(name, var, bool, perm); Maybe we want to make @config just a boolean initializer? e.g. something like #define module_param_config_on_off(name, var, perm, on_off) \ static bool var = on_off; \ module_param_named(name, var, bool, perm); so that the caller does IS_ENABLED() or whatever that's necessary? It just seems a bit too restricted. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 21/32] wireless: use %*pb[l] to print bitmaps including cpumasks and nodemasks
printk and friends can now formap bitmaps using '%*pb[l]'. cpumask
and nodemask also provide cpumask_pr_args() and nodemask_pr_args()
respectively which can be used to generate the two printf arguments
necessary to format the specified cpu/nodemask.
This patch is dependent on the following two patches.
lib/vsprintf: implement bitmap printing through '%*pb[l]'
cpumask, nodemask: implement cpumask/nodemask_pr_args()
Please wait till the forementioned patches are merged to mainline
before applying to subsystem trees.
Signed-off-by: Tejun Heo
Cc: Andrew Morton
Cc: "John W. Linville"
Cc: linux-wireless@vger.kernel.org
---
drivers/net/wireless/ath/ath9k/htc_drv_debug.c | 23 ++-
drivers/net/wireless/ath/carl9170/debug.c | 24 ++--
2 files changed, 12 insertions(+), 35 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c
b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c
index 8cef1ed..dc79afd 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c
@@ -291,26 +291,15 @@ static ssize_t read_file_slot(struct file *file, char
__user *user_buf,
{
struct ath9k_htc_priv *priv = file->private_data;
char buf[512];
- unsigned int len = 0;
+ unsigned int len;
spin_lock_bh(&priv->tx.tx_lock);
-
- len += scnprintf(buf + len, sizeof(buf) - len, "TX slot bitmap : ");
-
- len += bitmap_scnprintf(buf + len, sizeof(buf) - len,
- priv->tx.tx_slot, MAX_TX_BUF_NUM);
-
- len += scnprintf(buf + len, sizeof(buf) - len, "\n");
-
- len += scnprintf(buf + len, sizeof(buf) - len,
-"Used slots : %d\n",
-bitmap_weight(priv->tx.tx_slot, MAX_TX_BUF_NUM));
-
+ len = scnprintf(buf, sizeof(buf),
+ "TX slot bitmap : %*pb\n"
+ "Used slots : %d\n",
+ MAX_TX_BUF_NUM, priv->tx.tx_slot,
+ bitmap_weight(priv->tx.tx_slot, MAX_TX_BUF_NUM));
spin_unlock_bh(&priv->tx.tx_lock);
-
- if (len > sizeof(buf))
- len = sizeof(buf);
-
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
diff --git a/drivers/net/wireless/ath/carl9170/debug.c
b/drivers/net/wireless/ath/carl9170/debug.c
index 1c0af9c..6808db4 100644
--- a/drivers/net/wireless/ath/carl9170/debug.c
+++ b/drivers/net/wireless/ath/carl9170/debug.c
@@ -214,14 +214,10 @@ DEBUGFS_DECLARE_RO_FILE(name, _read_bufsize)
static char *carl9170_debugfs_mem_usage_read(struct ar9170 *ar, char *buf,
size_t bufsize, ssize_t *len)
{
- ADD(buf, *len, bufsize, "jar: [");
-
spin_lock_bh(&ar->mem_lock);
- *len += bitmap_scnprintf(&buf[*len], bufsize - *len,
- ar->mem_bitmap, ar->fw.mem_blocks);
-
- ADD(buf, *len, bufsize, "]\n");
+ ADD(buf, *len, bufsize, "jar: [%*pb]\n",
+ ar->fw.mem_blocks, ar->mem_bitmap);
ADD(buf, *len, bufsize, "cookies: used:%3d / total:%3d, allocs:%d\n",
bitmap_weight(ar->mem_bitmap, ar->fw.mem_blocks),
@@ -316,17 +312,13 @@ static char *carl9170_debugfs_ampdu_state_read(struct
ar9170 *ar, char *buf,
cnt, iter->tid, iter->bsn, iter->snx, iter->hsn,
iter->max, iter->state, iter->counter);
- ADD(buf, *len, bufsize, "\tWindow: [");
-
- *len += bitmap_scnprintf(&buf[*len], bufsize - *len,
- iter->bitmap, CARL9170_BAW_BITS);
+ ADD(buf, *len, bufsize, "\tWindow: [%*pb,W]\n",
+ CARL9170_BAW_BITS, iter->bitmap);
#define BM_STR_OFF(offset) \
((CARL9170_BAW_BITS - (offset) - 1) / 4 + \
(CARL9170_BAW_BITS - (offset) - 1) / 32 + 1)
- ADD(buf, *len, bufsize, ",W]\n");
-
offset = BM_STR_OFF(0);
ADD(buf, *len, bufsize, "\tBase Seq: %*s\n", offset, "T");
@@ -448,12 +440,8 @@ static char *carl9170_debugfs_vif_dump_read(struct ar9170
*ar, char *buf,
ADD(buf, *len, bufsize, "registered VIFs:%d \\ %d\n",
ar->vifs, ar->fw.vif_num);
- ADD(buf, *len, bufsize, "VIF bitmap: [");
-
- *len += bitmap_scnprintf(&buf[*len], bufsize - *len,
-&ar->vif_bitmap, ar->fw.vif_num);
-
- ADD(buf, *len, bufsize, "]\n");
+ ADD(buf, *len, bufsize, "VIF bitmap: [%*pb]\n",
+ ar->fw.vif_num, &ar->vif_bitmap);
rcu_read_lock();
list_for_each_entry_rcu(iter, &ar->vif_list, list) {
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
