Re: [PATCH v3 1/3] dax: Add an enum for specifying dax wakup mode
On Wed, Apr 21, 2021 at 05:16:24PM +0100, Matthew Wilcox wrote:
> On Wed, Apr 21, 2021 at 11:56:31AM -0400, Vivek Goyal wrote:
> > +/**
> > + * enum dax_entry_wake_mode: waitqueue wakeup toggle
>
> s/toggle/behaviour/ ?
Will do.
>
> > + * @WAKE_NEXT: wake only the first waiter in the waitqueue
> > + * @WAKE_ALL: wake all waiters in the waitqueue
> > + */
> > +enum dax_entry_wake_mode {
> > + WAKE_NEXT,
> > + WAKE_ALL,
> > +};
> > +
> > static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
> > void *entry, struct exceptional_entry_key *key)
> > {
> > @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(w
> > * The important information it's conveying is whether the entry at
> > * this index used to be a PMD entry.
> > */
> > -static void dax_wake_entry(struct xa_state *xas, void *entry, bool
> > wake_all)
> > +static void dax_wake_entry(struct xa_state *xas, void *entry,
> > + enum dax_entry_wake_mode mode)
>
> It's an awfully verbose name. 'dax_wake_mode'?
Sure. Will change.
Vivek
>
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v3 1/3] dax: Add an enum for specifying dax wakup mode
On Wed, Apr 21, 2021 at 11:56:31AM -0400, Vivek Goyal wrote:
> +/**
> + * enum dax_entry_wake_mode: waitqueue wakeup toggle
s/toggle/behaviour/ ?
> + * @WAKE_NEXT: wake only the first waiter in the waitqueue
> + * @WAKE_ALL: wake all waiters in the waitqueue
> + */
> +enum dax_entry_wake_mode {
> + WAKE_NEXT,
> + WAKE_ALL,
> +};
> +
> static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
> void *entry, struct exceptional_entry_key *key)
> {
> @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(w
> * The important information it's conveying is whether the entry at
> * this index used to be a PMD entry.
> */
> -static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all)
> +static void dax_wake_entry(struct xa_state *xas, void *entry,
> +enum dax_entry_wake_mode mode)
It's an awfully verbose name. 'dax_wake_mode'?
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v3 1/3] dax: Add an enum for specifying dax wakup mode
On Wed, Apr 21, 2021 at 11:24:40AM +0200, Jan Kara wrote:
> On Mon 19-04-21 17:36:34, Vivek Goyal wrote:
> > Dan mentioned that he is not very fond of passing around a boolean
> > true/false
> > to specify if only next waiter should be woken up or all waiters should be
> > woken up. He instead prefers that we introduce an enum and make it very
> > explicity at the callsite itself. Easier to read code.
> >
> > This patch should not introduce any change of behavior.
> >
> > Suggested-by: Dan Williams
> > Signed-off-by: Vivek Goyal
> > ---
> > fs/dax.c | 23 +--
> > 1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/dax.c b/fs/dax.c
> > index b3d27fdc6775..00978d0838b1 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue {
> > struct exceptional_entry_key key;
> > };
> >
> > +/**
> > + * enum dax_entry_wake_mode: waitqueue wakeup toggle
> > + * @WAKE_NEXT: entry was not mutated
> > + * @WAKE_ALL: entry was invalidated, or resized
>
> Let's document the constants in terms of what they do, not when they are
> expected to be called. So something like:
>
> @WAKE_NEXT: wake only the first waiter in the waitqueue
> @WAKE_ALL: wake all waiters in the waitqueue
>
> Otherwise the patch looks good so feel free to add:
>
> Reviewed-by: Jan Kara
>
Hi Jan,
Here is the updated patch based on your feedback.
Thanks
Vivek
Subject: dax: Add an enum for specifying dax wakup mode
Dan mentioned that he is not very fond of passing around a boolean true/false
to specify if only next waiter should be woken up or all waiters should be
woken up. He instead prefers that we introduce an enum and make it very
explicity at the callsite itself. Easier to read code.
This patch should not introduce any change of behavior.
Suggested-by: Dan Williams
Signed-off-by: Vivek Goyal
---
fs/dax.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)
Index: redhat-linux/fs/dax.c
===
--- redhat-linux.orig/fs/dax.c 2021-04-21 11:51:04.716289502 -0400
+++ redhat-linux/fs/dax.c 2021-04-21 11:52:10.298010850 -0400
@@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue {
struct exceptional_entry_key key;
};
+/**
+ * enum dax_entry_wake_mode: waitqueue wakeup toggle
+ * @WAKE_NEXT: wake only the first waiter in the waitqueue
+ * @WAKE_ALL: wake all waiters in the waitqueue
+ */
+enum dax_entry_wake_mode {
+ WAKE_NEXT,
+ WAKE_ALL,
+};
+
static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
void *entry, struct exceptional_entry_key *key)
{
@@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(w
* The important information it's conveying is whether the entry at
* this index used to be a PMD entry.
*/
-static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all)
+static void dax_wake_entry(struct xa_state *xas, void *entry,
+ enum dax_entry_wake_mode mode)
{
struct exceptional_entry_key key;
wait_queue_head_t *wq;
@@ -196,7 +207,7 @@ static void dax_wake_entry(struct xa_sta
* must be in the waitqueue and the following check will see them.
*/
if (waitqueue_active(wq))
- __wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, );
+ __wake_up(wq, TASK_NORMAL, mode == WAKE_ALL ? 0 : 1, );
}
/*
@@ -268,7 +279,7 @@ static void put_unlocked_entry(struct xa
{
/* If we were the only waiter woken, wake the next one */
if (entry && !dax_is_conflict(entry))
- dax_wake_entry(xas, entry, false);
+ dax_wake_entry(xas, entry, WAKE_NEXT);
}
/*
@@ -286,7 +297,7 @@ static void dax_unlock_entry(struct xa_s
old = xas_store(xas, entry);
xas_unlock_irq(xas);
BUG_ON(!dax_is_locked(old));
- dax_wake_entry(xas, entry, false);
+ dax_wake_entry(xas, entry, WAKE_NEXT);
}
/*
@@ -524,7 +535,7 @@ retry:
dax_disassociate_entry(entry, mapping, false);
xas_store(xas, NULL); /* undo the PMD join */
- dax_wake_entry(xas, entry, true);
+ dax_wake_entry(xas, entry, WAKE_ALL);
mapping->nrexceptional--;
entry = NULL;
xas_set(xas, index);
@@ -937,7 +948,7 @@ static int dax_writeback_one(struct xa_s
xas_lock_irq(xas);
xas_store(xas, entry);
xas_clear_mark(xas, PAGECACHE_TAG_DIRTY);
- dax_wake_entry(xas, entry, false);
+ dax_wake_entry(xas, entry, WAKE_NEXT);
trace_dax_writeback_one(mapping->host, index, count);
return ret;
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v3 1/3] dax: Add an enum for specifying dax wakup mode
On Mon 19-04-21 17:36:34, Vivek Goyal wrote:
> Dan mentioned that he is not very fond of passing around a boolean true/false
> to specify if only next waiter should be woken up or all waiters should be
> woken up. He instead prefers that we introduce an enum and make it very
> explicity at the callsite itself. Easier to read code.
>
> This patch should not introduce any change of behavior.
>
> Suggested-by: Dan Williams
> Signed-off-by: Vivek Goyal
> ---
> fs/dax.c | 23 +--
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index b3d27fdc6775..00978d0838b1 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue {
> struct exceptional_entry_key key;
> };
>
> +/**
> + * enum dax_entry_wake_mode: waitqueue wakeup toggle
> + * @WAKE_NEXT: entry was not mutated
> + * @WAKE_ALL: entry was invalidated, or resized
Let's document the constants in terms of what they do, not when they are
expected to be called. So something like:
@WAKE_NEXT: wake only the first waiter in the waitqueue
@WAKE_ALL: wake all waiters in the waitqueue
Otherwise the patch looks good so feel free to add:
Reviewed-by: Jan Kara
Honza
> + */
> +enum dax_entry_wake_mode {
> + WAKE_NEXT,
> + WAKE_ALL,
> +};
> +
> static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
> void *entry, struct exceptional_entry_key *key)
> {
> @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(wait_queue_entry_t
> *wait,
> * The important information it's conveying is whether the entry at
> * this index used to be a PMD entry.
> */
> -static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all)
> +static void dax_wake_entry(struct xa_state *xas, void *entry,
> +enum dax_entry_wake_mode mode)
> {
> struct exceptional_entry_key key;
> wait_queue_head_t *wq;
> @@ -196,7 +207,7 @@ static void dax_wake_entry(struct xa_state *xas, void
> *entry, bool wake_all)
>* must be in the waitqueue and the following check will see them.
>*/
> if (waitqueue_active(wq))
> - __wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, );
> + __wake_up(wq, TASK_NORMAL, mode == WAKE_ALL ? 0 : 1, );
> }
>
> /*
> @@ -268,7 +279,7 @@ static void put_unlocked_entry(struct xa_state *xas, void
> *entry)
> {
> /* If we were the only waiter woken, wake the next one */
> if (entry && !dax_is_conflict(entry))
> - dax_wake_entry(xas, entry, false);
> + dax_wake_entry(xas, entry, WAKE_NEXT);
> }
>
> /*
> @@ -286,7 +297,7 @@ static void dax_unlock_entry(struct xa_state *xas, void
> *entry)
> old = xas_store(xas, entry);
> xas_unlock_irq(xas);
> BUG_ON(!dax_is_locked(old));
> - dax_wake_entry(xas, entry, false);
> + dax_wake_entry(xas, entry, WAKE_NEXT);
> }
>
> /*
> @@ -524,7 +535,7 @@ static void *grab_mapping_entry(struct xa_state *xas,
>
> dax_disassociate_entry(entry, mapping, false);
> xas_store(xas, NULL); /* undo the PMD join */
> - dax_wake_entry(xas, entry, true);
> + dax_wake_entry(xas, entry, WAKE_ALL);
> mapping->nrexceptional--;
> entry = NULL;
> xas_set(xas, index);
> @@ -937,7 +948,7 @@ static int dax_writeback_one(struct xa_state *xas, struct
> dax_device *dax_dev,
> xas_lock_irq(xas);
> xas_store(xas, entry);
> xas_clear_mark(xas, PAGECACHE_TAG_DIRTY);
> - dax_wake_entry(xas, entry, false);
> + dax_wake_entry(xas, entry, WAKE_NEXT);
>
> trace_dax_writeback_one(mapping->host, index, count);
> return ret;
> --
> 2.25.4
>
--
Jan Kara
SUSE Labs, CR
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [Virtio-fs] [PATCH v3 1/3] dax: Add an enum for specifying dax wakup mode
On Mon, 19 Apr 2021 17:36:34 -0400
Vivek Goyal wrote:
> Dan mentioned that he is not very fond of passing around a boolean true/false
> to specify if only next waiter should be woken up or all waiters should be
> woken up. He instead prefers that we introduce an enum and make it very
> explicity at the callsite itself. Easier to read code.
>
> This patch should not introduce any change of behavior.
>
> Suggested-by: Dan Williams
> Signed-off-by: Vivek Goyal
> ---
Reviewed-by: Greg Kurz
> fs/dax.c | 23 +--
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index b3d27fdc6775..00978d0838b1 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue {
> struct exceptional_entry_key key;
> };
>
> +/**
> + * enum dax_entry_wake_mode: waitqueue wakeup toggle
> + * @WAKE_NEXT: entry was not mutated
> + * @WAKE_ALL: entry was invalidated, or resized
> + */
> +enum dax_entry_wake_mode {
> + WAKE_NEXT,
> + WAKE_ALL,
> +};
> +
> static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
> void *entry, struct exceptional_entry_key *key)
> {
> @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(wait_queue_entry_t
> *wait,
> * The important information it's conveying is whether the entry at
> * this index used to be a PMD entry.
> */
> -static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all)
> +static void dax_wake_entry(struct xa_state *xas, void *entry,
> +enum dax_entry_wake_mode mode)
> {
> struct exceptional_entry_key key;
> wait_queue_head_t *wq;
> @@ -196,7 +207,7 @@ static void dax_wake_entry(struct xa_state *xas, void
> *entry, bool wake_all)
>* must be in the waitqueue and the following check will see them.
>*/
> if (waitqueue_active(wq))
> - __wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, );
> + __wake_up(wq, TASK_NORMAL, mode == WAKE_ALL ? 0 : 1, );
> }
>
> /*
> @@ -268,7 +279,7 @@ static void put_unlocked_entry(struct xa_state *xas, void
> *entry)
> {
> /* If we were the only waiter woken, wake the next one */
> if (entry && !dax_is_conflict(entry))
> - dax_wake_entry(xas, entry, false);
> + dax_wake_entry(xas, entry, WAKE_NEXT);
> }
>
> /*
> @@ -286,7 +297,7 @@ static void dax_unlock_entry(struct xa_state *xas, void
> *entry)
> old = xas_store(xas, entry);
> xas_unlock_irq(xas);
> BUG_ON(!dax_is_locked(old));
> - dax_wake_entry(xas, entry, false);
> + dax_wake_entry(xas, entry, WAKE_NEXT);
> }
>
> /*
> @@ -524,7 +535,7 @@ static void *grab_mapping_entry(struct xa_state *xas,
>
> dax_disassociate_entry(entry, mapping, false);
> xas_store(xas, NULL); /* undo the PMD join */
> - dax_wake_entry(xas, entry, true);
> + dax_wake_entry(xas, entry, WAKE_ALL);
> mapping->nrexceptional--;
> entry = NULL;
> xas_set(xas, index);
> @@ -937,7 +948,7 @@ static int dax_writeback_one(struct xa_state *xas, struct
> dax_device *dax_dev,
> xas_lock_irq(xas);
> xas_store(xas, entry);
> xas_clear_mark(xas, PAGECACHE_TAG_DIRTY);
> - dax_wake_entry(xas, entry, false);
> + dax_wake_entry(xas, entry, WAKE_NEXT);
>
> trace_dax_writeback_one(mapping->host, index, count);
> return ret;
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[PATCH v3 1/3] dax: Add an enum for specifying dax wakup mode
Dan mentioned that he is not very fond of passing around a boolean true/false
to specify if only next waiter should be woken up or all waiters should be
woken up. He instead prefers that we introduce an enum and make it very
explicity at the callsite itself. Easier to read code.
This patch should not introduce any change of behavior.
Suggested-by: Dan Williams
Signed-off-by: Vivek Goyal
---
fs/dax.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index b3d27fdc6775..00978d0838b1 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue {
struct exceptional_entry_key key;
};
+/**
+ * enum dax_entry_wake_mode: waitqueue wakeup toggle
+ * @WAKE_NEXT: entry was not mutated
+ * @WAKE_ALL: entry was invalidated, or resized
+ */
+enum dax_entry_wake_mode {
+ WAKE_NEXT,
+ WAKE_ALL,
+};
+
static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
void *entry, struct exceptional_entry_key *key)
{
@@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(wait_queue_entry_t
*wait,
* The important information it's conveying is whether the entry at
* this index used to be a PMD entry.
*/
-static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all)
+static void dax_wake_entry(struct xa_state *xas, void *entry,
+ enum dax_entry_wake_mode mode)
{
struct exceptional_entry_key key;
wait_queue_head_t *wq;
@@ -196,7 +207,7 @@ static void dax_wake_entry(struct xa_state *xas, void
*entry, bool wake_all)
* must be in the waitqueue and the following check will see them.
*/
if (waitqueue_active(wq))
- __wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, );
+ __wake_up(wq, TASK_NORMAL, mode == WAKE_ALL ? 0 : 1, );
}
/*
@@ -268,7 +279,7 @@ static void put_unlocked_entry(struct xa_state *xas, void
*entry)
{
/* If we were the only waiter woken, wake the next one */
if (entry && !dax_is_conflict(entry))
- dax_wake_entry(xas, entry, false);
+ dax_wake_entry(xas, entry, WAKE_NEXT);
}
/*
@@ -286,7 +297,7 @@ static void dax_unlock_entry(struct xa_state *xas, void
*entry)
old = xas_store(xas, entry);
xas_unlock_irq(xas);
BUG_ON(!dax_is_locked(old));
- dax_wake_entry(xas, entry, false);
+ dax_wake_entry(xas, entry, WAKE_NEXT);
}
/*
@@ -524,7 +535,7 @@ static void *grab_mapping_entry(struct xa_state *xas,
dax_disassociate_entry(entry, mapping, false);
xas_store(xas, NULL); /* undo the PMD join */
- dax_wake_entry(xas, entry, true);
+ dax_wake_entry(xas, entry, WAKE_ALL);
mapping->nrexceptional--;
entry = NULL;
xas_set(xas, index);
@@ -937,7 +948,7 @@ static int dax_writeback_one(struct xa_state *xas, struct
dax_device *dax_dev,
xas_lock_irq(xas);
xas_store(xas, entry);
xas_clear_mark(xas, PAGECACHE_TAG_DIRTY);
- dax_wake_entry(xas, entry, false);
+ dax_wake_entry(xas, entry, WAKE_NEXT);
trace_dax_writeback_one(mapping->host, index, count);
return ret;
--
2.25.4
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
