percpu_ref, RCU: Audit RCU usages in percpu_ref users

rvt_mregion uses percpu_ref for reference counting and RCU to protect accesses from lkey_table. When a rvt_mregion needs to be freed, it first gets unregistered from lkey_table and then rvt_check_refs() is called to wait for in-flight usages before the rvt_mregion is freed.

rvt_check_refs() seems to have a couple issues.

* It has a fast exit path which tests percpu_ref_is_zero(). However, a percpu_ref reading zero doesn't mean that the object can be released. In fact, the ->release() callback might not even have started executing yet. Proceeding with freeing can lead to use-after-free.

* lkey_table is RCU protected but there is no RCU grace period in the free path. percpu_ref uses RCU internally but it's sched-RCU whose grace periods are different from regular RCU. Also, it generally isn't a good idea to depend on internal behaviors like this.

To address the above issues, this patch removes the fast exit and adds an explicit synchronize_rcu().

Signed-off-by Tejun Heo <[email protected]>

Acked-by Dennis Dalessandro <[email protected]>

Cc Mike Marciniszyn <[email protected]>

Cc [email protected]

Cc Linus Torvalds <[email protected]>

 drivers/infiniband/sw/rdmavt/mr.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

hmm_devmem_find() requires rcu_read_lock_held() but there's nothing which actually uses the RCU protection. The only caller is hmm_devmem_pages_create() which already grabs the mutex and does superflous rcu_read_lock/unlock() around the function.

This doesn't add anything and just adds to confusion. Remove the RCU protection and open-code the radix tree lookup. If this needs to become more sophisticated in the future, let's add them back when necessary.

Signed-off-by Tejun Heo <[email protected]>

Reviewed-by Jérôme Glisse <[email protected]>

Cc [email protected]

Cc Linus Torvalds <[email protected]>

---
Hello,

Jérôme, how do you want to route this patch? If you prefer, I can route it together with other patches.

Thanks.

 mm/hmm.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

mm/hmm.c +2 -10

index 320545b98..d4627c5 100644
@@ -845,13 +845,6 @@ static void hmm_devmem_release(struct device *dev, void *data)
 	hmm_devmem_radix_release(resource);
 }
 
-static struct hmm_devmem *hmm_devmem_find(resource_size_t phys)
-{
-	WARN_ON_ONCE(!rcu_read_lock_held());
-
-	return radix_tree_lookup(&hmm_devmem_radix, phys >> PA_SECTION_SHIFT);
-}
-
 static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
 {
 	resource_size_t key, align_start, align_size, align_end;

@@ -892,9 +885,8 @@ static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
 	for (key = align_start; key <= align_end; key += PA_SECTION_SIZE) {
 		struct hmm_devmem *dup;
 
-		rcu_read_lock();
-		dup = hmm_devmem_find(key);
-		rcu_read_unlock();
+		dup = radix_tree_lookup(&hmm_devmem_radix,
+					key >> PA_SECTION_SHIFT);
 		if (dup) {
 			dev_err(device, "%s: collides with mapping for %s\n",
 				__func__, dev_name(dup->device));

Signature · 1 line

2.9.5

Hello, Andrew.

Do you mind picking up the following patch? I can't find a good tree to route this through. The raw patch can be found at

https://marc.info/?l=linux-mm&m=152105674112496&q=raw

Thank you very much.

Quoted 58 lines

hmm_devmem_find() requires rcu_read_lock_held() but there's nothing which actually uses the RCU protection. The only caller is hmm_devmem_pages_create() which already grabs the mutex and does superflous rcu_read_lock/unlock() around the function.

This doesn't add anything and just adds to confusion. Remove the RCU protection and open-code the radix tree lookup. If this needs to become more sophisticated in the future, let's add them back when necessary.

Signed-off-by Tejun Heo <[email protected]>

Reviewed-by Jérôme Glisse <[email protected]>

Cc [email protected]

Cc Linus Torvalds <[email protected]>

---
Hello,

Jérôme, how do you want to route this patch? If you prefer, I can route it together with other patches.

Thanks.

 mm/hmm.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

mm/hmm.c +2 -10

index 320545b98..d4627c5 100644
@@ -845,13 +845,6 @@ static void hmm_devmem_release(struct device *dev, void *data)
 	hmm_devmem_radix_release(resource);
 }
 
-static struct hmm_devmem *hmm_devmem_find(resource_size_t phys)
-{
-	WARN_ON_ONCE(!rcu_read_lock_held());
-
-	return radix_tree_lookup(&hmm_devmem_radix, phys >> PA_SECTION_SHIFT);
-}
-
 static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
 {
 	resource_size_t key, align_start, align_size, align_end;

@@ -892,9 +885,8 @@ static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
 	for (key = align_start; key <= align_end; key += PA_SECTION_SIZE) {
 		struct hmm_devmem *dup;
 
-		rcu_read_lock();
-		dup = hmm_devmem_find(key);
-		rcu_read_unlock();
+		dup = radix_tree_lookup(&hmm_devmem_radix,
+					key >> PA_SECTION_SHIFT);
 		if (dup) {
 			dev_err(device, "%s: collides with mapping for %s\n",
 				__func__, dev_name(dup->device));

Signature · 1 line

2.9.5

Signature · 1 line

tejun

This patch makes it clear that the grace periods are percpu_ref's internal implementation detail and can't be depended upon by the users.

There are cases where RCU callback needs to be bounced to a sleepable context. This is currently done by the RCU callback queueing a work item, which can be cumbersome to write and confusing to read.

This patch introduces rcu_work, a workqueue work variant which gets executed after a RCU grace period, and converts the open coded bouncing in fs/aio and kernel/cgroup.

v3: Dropped queue_rcu_work_on(). Documented rcu grace period behavior
after queue_rcu_work().

v2: Use rcu_barrier() instead of synchronize_rcu() to wait for
completion of previously queued rcu callback as per Paul.

Signed-off-by Tejun Heo <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 include/linux/workqueue.h | 23 ++++++++++++++++++++
 kernel/workqueue.c        | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

include/linux/workqueue.h +23 -0

index bc0cda1..d026f8f 100644
@@ -13,6 +13,7 @@
 #include <linux/threads.h>
 #include <linux/atomic.h>
 #include <linux/cpumask.h>
+#include <linux/rcupdate.h>
 
 struct workqueue_struct;
 

@@ -120,6 +121,14 @@ struct delayed_work {
 	int cpu;
 };
 
+struct rcu_work {
+	struct work_struct work;
+	struct rcu_head rcu;
+
+	/* target workqueue ->rcu uses to queue ->work */
+	struct workqueue_struct *wq;
+};
+
 /**
  * struct workqueue_attrs - A struct for workqueue attributes.
  *

@@ -151,6 +160,11 @@ static inline struct delayed_work *to_delayed_work(struct work_struct *work)
 	return container_of(work, struct delayed_work, work);
 }
 
+static inline struct rcu_work *to_rcu_work(struct work_struct *work)
+{
+	return container_of(work, struct rcu_work, work);
+}
+
 struct execute_work {
 	struct work_struct work;
 };

@@ -266,6 +280,12 @@ static inline unsigned int work_static(struct work_struct *work) { return 0; }
 #define INIT_DEFERRABLE_WORK_ONSTACK(_work, _func)			\
 	__INIT_DELAYED_WORK_ONSTACK(_work, _func, TIMER_DEFERRABLE)
 
+#define INIT_RCU_WORK(_work, _func)					\
+	INIT_WORK(&(_work)->work, (_func))
+
+#define INIT_RCU_WORK_ONSTACK(_work, _func)				\
+	INIT_WORK_ONSTACK(&(_work)->work, (_func))
+
 /**
  * work_pending - Find out whether a work item is currently pending
  * @work: The work item in question

@@ -447,6 +467,7 @@ extern bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
 			struct delayed_work *work, unsigned long delay);
 extern bool mod_delayed_work_on(int cpu, struct workqueue_struct *wq,
 			struct delayed_work *dwork, unsigned long delay);
+extern bool queue_rcu_work(struct workqueue_struct *wq, struct rcu_work *rwork);
 
 extern void flush_workqueue(struct workqueue_struct *wq);
 extern void drain_workqueue(struct workqueue_struct *wq);

@@ -463,6 +484,8 @@ extern bool flush_delayed_work(struct delayed_work *dwork);
 extern bool cancel_delayed_work(struct delayed_work *dwork);
 extern bool cancel_delayed_work_sync(struct delayed_work *dwork);
 
+extern bool flush_rcu_work(struct rcu_work *rwork);
+
 extern void workqueue_set_max_active(struct workqueue_struct *wq,
 				     int max_active);
 extern struct work_struct *current_work(void);

kernel/workqueue.c +54 -0

index bb9a519..7df85fa 100644
@@ -1604,6 +1604,40 @@ bool mod_delayed_work_on(int cpu, struct workqueue_struct *wq,
 }
 EXPORT_SYMBOL_GPL(mod_delayed_work_on);
 
+static void rcu_work_rcufn(struct rcu_head *rcu)
+{
+	struct rcu_work *rwork = container_of(rcu, struct rcu_work, rcu);
+
+	/* read the comment in __queue_work() */
+	local_irq_disable();
+	__queue_work(WORK_CPU_UNBOUND, rwork->wq, &rwork->work);
+	local_irq_enable();
+}
+
+/**
+ * queue_rcu_work - queue work after a RCU grace period
+ * @wq: workqueue to use
+ * @rwork: work to queue
+ *
+ * Return: %false if @rwork was already pending, %true otherwise.  Note
+ * that a full RCU grace period is guaranteed only after a %true return.
+ * While @rwork is guarnateed to be executed after a %false return, the
+ * execution may happen before a full RCU grace period has passed.
+ */
+bool queue_rcu_work(struct workqueue_struct *wq, struct rcu_work *rwork)
+{
+	struct work_struct *work = &rwork->work;
+
+	if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
+		rwork->wq = wq;
+		call_rcu(&rwork->rcu, rcu_work_rcufn);
+		return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(queue_rcu_work);
+
 /**
  * worker_enter_idle - enter idle state
  * @worker: worker which is entering idle state

@@ -3001,6 +3035,26 @@ bool flush_delayed_work(struct delayed_work *dwork)
 }
 EXPORT_SYMBOL(flush_delayed_work);
 
+/**
+ * flush_rcu_work - wait for a rwork to finish executing the last queueing
+ * @rwork: the rcu work to flush
+ *
+ * Return:
+ * %true if flush_rcu_work() waited for the work to finish execution,
+ * %false if it was already idle.
+ */
+bool flush_rcu_work(struct rcu_work *rwork)
+{
+	if (test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&rwork->work))) {
+		rcu_barrier();
+		flush_work(&rwork->work);
+		return true;
+	} else {
+		return flush_work(&rwork->work);
+	}
+}
+EXPORT_SYMBOL(flush_rcu_work);
+
 static bool __cancel_work(struct work_struct *work, bool is_dwork)
 {
 	unsigned long flags;

Signature · 1 line

2.9.5

There are cases where RCU callback needs to be bounced to a sleepable context. This is currently done by the RCU callback queueing a work item, which can be cumbersome to write and confusing to read.

This patch introduces rcu_work, a workqueue work variant which gets executed after a RCU grace period, and converts the open coded bouncing in fs/aio and kernel/cgroup.

v3: Dropped queue_rcu_work_on(). Documented rcu grace period behavior
after queue_rcu_work().

v2: Use rcu_barrier() instead of synchronize_rcu() to wait for
completion of previously queued rcu callback as per Paul.

Signed-off-by Tejun Heo <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 include/linux/workqueue.h | 23 ++++++++++++++++++++
 kernel/workqueue.c        | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

include/linux/workqueue.h +23 -0

index bc0cda1..d026f8f 100644
@@ -13,6 +13,7 @@
 #include <linux/threads.h>
 #include <linux/atomic.h>
 #include <linux/cpumask.h>
+#include <linux/rcupdate.h>
 
 struct workqueue_struct;
 

@@ -120,6 +121,14 @@ struct delayed_work {
        int cpu;
 };
 
+struct rcu_work {
+       struct work_struct work;
+       struct rcu_head rcu;
+
+       /* target workqueue ->rcu uses to queue ->work */
+       struct workqueue_struct *wq;
+};
+
 /**
  * struct workqueue_attrs - A struct for workqueue attributes.
  *

@@ -151,6 +160,11 @@ static inline struct delayed_work *to_delayed_work(struct work_struct *work)
        return container_of(work, struct delayed_work, work);
 }
 
+static inline struct rcu_work *to_rcu_work(struct work_struct *work)
+{
+       return container_of(work, struct rcu_work, work);
+}
+
 struct execute_work {
        struct work_struct work;
 };

@@ -266,6 +280,12 @@ static inline unsigned int work_static(struct work_struct *work) { return 0; }
 #define INIT_DEFERRABLE_WORK_ONSTACK(_work, _func)                     \
        __INIT_DELAYED_WORK_ONSTACK(_work, _func, TIMER_DEFERRABLE)
 
+#define INIT_RCU_WORK(_work, _func)                                    \
+       INIT_WORK(&(_work)->work, (_func))
+
+#define INIT_RCU_WORK_ONSTACK(_work, _func)                            \
+       INIT_WORK_ONSTACK(&(_work)->work, (_func))
+
 /**
  * work_pending - Find out whether a work item is currently pending
  * @work: The work item in question

@@ -447,6 +467,7 @@ extern bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
                        struct delayed_work *work, unsigned long delay);
 extern bool mod_delayed_work_on(int cpu, struct workqueue_struct *wq,
                        struct delayed_work *dwork, unsigned long delay);
+extern bool queue_rcu_work(struct workqueue_struct *wq, struct rcu_work *rwork);
 
 extern void flush_workqueue(struct workqueue_struct *wq);
 extern void drain_workqueue(struct workqueue_struct *wq);

@@ -463,6 +484,8 @@ extern bool flush_delayed_work(struct delayed_work *dwork);
 extern bool cancel_delayed_work(struct delayed_work *dwork);
 extern bool cancel_delayed_work_sync(struct delayed_work *dwork);
 
+extern bool flush_rcu_work(struct rcu_work *rwork);
+
 extern void workqueue_set_max_active(struct workqueue_struct *wq,
                                     int max_active);
 extern struct work_struct *current_work(void);

kernel/workqueue.c +10 -0

index bb9a519..7df85fa 100644
@@ -1604,6 +1604,40 @@ bool mod_delayed_work_on(int cpu, struct workqueue_struct *wq,
 }
 EXPORT_SYMBOL_GPL(mod_delayed_work_on);
 
+static void rcu_work_rcufn(struct rcu_head *rcu)
+{
+       struct rcu_work *rwork = container_of(rcu, struct rcu_work, rcu);
+
+       /* read the comment in __queue_work() */
+       local_irq_disable();
+       __queue_work(WORK_CPU_UNBOUND, rwork->wq, &rwork->work);
+       local_irq_enable();
+}
+

kernel/workqueue.c +18 -0

+bool queue_rcu_work(struct workqueue_struct *wq, struct rcu_work *rwork)
+{
+       struct work_struct *work = &rwork->work;
+
+       if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
+               rwork->wq = wq;
+               call_rcu(&rwork->rcu, rcu_work_rcufn);
+               return true;
+       }
+
+       return false;
+}
+EXPORT_SYMBOL(queue_rcu_work);
+
 /**
  * worker_enter_idle - enter idle state
  * @worker: worker which is entering idle state

@@ -3001,6 +3035,26 @@ bool flush_delayed_work(struct delayed_work *dwork)
 }
 EXPORT_SYMBOL(flush_delayed_work);
 
+/**
+ * flush_rcu_work - wait for a rwork to finish executing the last queueing
+ * @rwork: the rcu work to flush
+ *

kernel/workqueue.c +6 -1

+       } else {
+               return flush_work(&rwork->work);
+       }
+}
+EXPORT_SYMBOL(flush_rcu_work);
+
 static bool __cancel_work(struct work_struct *work, bool is_dwork)
 {
        unsigned long flags;
--

2.9.5

kernel/workqueue.c bool flush_delayed_work(struct delayed_work *dwork) · line 3051

Quoted 6 lines

kernel/workqueue.c +6 -0

+bool flush_rcu_work(struct rcu_work *rwork)
+{
+       if (test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&rwork->work))) {
+               rcu_barrier();
+               flush_work(&rwork->work);
+               return true;

A possible tiny improvement: check if it was already queued on wq. For example:

       if (test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&rwork->work))) {
               if (!flush_work(&rwork->work)) {
                      rcu_barrier();
                      flush_work(&rwork->work);
               }
               return true;

Hello, Lai.

Quoted 10 lines · Lai Jiangshan

kernel/workqueue.c bool flush_delayed_work(struct delayed_work *dwork) · line 3051

Quoted 6 lines

kernel/workqueue.c +6 -0

+bool flush_rcu_work(struct rcu_work *rwork)
+{
+       if (test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&rwork->work))) {
+               rcu_barrier();
+               flush_work(&rwork->work);
+               return true;

A possible tiny improvement: check if it was already queued on wq. For example:

Quoted 6 lines

       if (test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&rwork->work))) {
               if (!flush_work(&rwork->work)) {
                      rcu_barrier();
                      flush_work(&rwork->work);
               }
               return true;

But this breaks the guarantee that flush_work waits for the latest queueing instance. Please consider the following scenario.

1. rcu-work is queued
2. rcu-work starts executing
3. rcu-work is queued again
4. rcu-work is flushed
5. execution finishes
6. flush finishes
7. execution finishes

6 should happen after 7 but it didn't.

Thanks.

--
tejun

Workqueue now has rcu_work. Use it instead of open-coding rcu -> work item bouncing.

Signed-off-by Tejun Heo <[email protected]>

Workqueue now has rcu_work. Use it instead of open-coding rcu -> work item bouncing.

fs/aio.c struct kioctx {

Quoted 3 lines

fs/aio.c +1 -2

-	struct rcu_head		free_rcu;
-	struct work_struct	free_work;	/* see free_ioctx() */
+	struct rcu_work		free_rwork;	/* see free_ioctx() */

IIUC, you can't easily share rcu_work's, thus every kioctx needs its own ->free_rwork and this looks sub-optimal.

What do you think about the (untested) patch below?

Oleg.

Hey, Oleg.

Quoted 18 lines · Oleg Nesterov

fs/aio.c struct kioctx {

Quoted 3 lines

fs/aio.c +1 -2

-	struct rcu_head		free_rcu;
-	struct work_struct	free_work;	/* see free_ioctx() */
+	struct rcu_work		free_rwork;	/* see free_ioctx() */

IIUC, you can't easily share rcu_work's, thus every kioctx needs its own ->free_rwork and this looks sub-optimal.

What do you think about the (untested) patch below?

Oleg.

fs/aio.c +0 -0

@@ -115,8 +115,10 @@ struct kioctx {
 	struct page		**ring_pages;
 	long			nr_pages;
 

fs/aio.c struct kioctx { · line 121

Quoted 6 lines

fs/aio.c +4 -2

-	struct rcu_head		free_rcu;
-	struct work_struct	free_work;	/* see free_ioctx() */
+	union {
+		struct rcu_head		free_rcu;
+		struct llist_node	free_llist;
+	};

It really depends on how much we want to optimize. Do you think it matters enough?

Mostly I am asking because I do not really understand "[PATCH 6/8] RCU, workqueue: Implement rcu_work".

I mean, the code looks simple and correct but why does it play with WORK_STRUCT_PENDING_BIT? IOW, I do not see a "good" use-case when 2 or more queue_rcu_work()'s can use the same rwork and hit work_pending() == T. And what the caller should do if queue_rcu_work() returns false?

Hello,

Quoted 3 lines · Oleg Nesterov

Mostly I am asking because I do not really understand "[PATCH 6/8] RCU, workqueue: Implement rcu_work".

Quoted 4 lines

I mean, the code looks simple and correct but why does it play with WORK_STRUCT_PENDING_BIT? IOW, I do not see a "good" use-case when 2 or more queue_rcu_work()'s can use the same rwork and hit work_pending() == T. And what the caller should do if queue_rcu_work() returns false?

It's just following standard workqueue conventions.

We can try to
make it more specialized but then flush_rcu_work()'s behavior would have to different too and it gets confusing quickly. Unless there are overriding reasons to deviate, I'd like to keep it consistent.

OK. And I agree that the usage of WORK_STRUCT_PENDING_BIT in queue_rcu_work() is fine. If nothing else the kernel won't crash if you call queue_rcu_work() twice.

But why flush_rcu_work() can't simply do flush_work() ?

If WORK_STRUCT_PENDING_BIT was set by us (rcu_work_rcufn() succeeded) we do not need rcu_barrier(). Why should we care about other rcu callbacks?

If rcu_work_rcufn() fails and someone else sets PENDING, how this rcu_barrier() can help? We didn't even do call_rcu() in this case.

In short. Once flush_work() returns we know that rcu callback which queued this work is finished. It doesn't matter if it was fired by us or not. And if it was not fired by us, then rcu_barrier() doesn't imply a grace period anyway.

Quoted 4 lines

We can try to
make it more specialized but then flush_rcu_work()'s behavior would have to different too and it gets confusing quickly. Unless there are overriding reasons to deviate, I'd like to keep it consistent.

Perhaps this all is consistent, but I fail to understand this API :/

And again, at least for fs/aio.c it doesn't offer too much but sub-optimal compared to call_rcu() + schedule_work() by hand.

Quoted 9 lines · Oleg Nesterov

But why flush_rcu_work() can't simply do flush_work() ?

If WORK_STRUCT_PENDING_BIT was set by us (rcu_work_rcufn() succeeded) we do not need rcu_barrier(). Why should we care about other rcu callbacks?

If rcu_work_rcufn() fails and someone else sets PENDING, how this rcu_barrier() can help? We didn't even do call_rcu() in this case.

Quoted 3 lines

In short. Once flush_work() returns we know that rcu callback which queued this work is finished. It doesn't matter if it was fired by us or not. And if it was not fired by us, then rcu_barrier() doesn't imply a grace period anyway.

flush_*work() guarantees to wait for the completion of the latest instance of the work item which was visible to the caller. We can't guarantee that w/o rcu_barrier().

Quoted 2 lines

And again, at least for fs/aio.c it doesn't offer too much but sub-optimal compared to call_rcu() + schedule_work() by hand.

Sure, this isn't about performance. It's about making code less painful on the eyes. If performance matters, we sure can hand-craft things, which doesn't seem to be the case, right?

Quoted 3 lines

flush_*work() guarantees to wait for the completion of the latest instance of the work item which was visible to the caller. We can't guarantee that w/o rcu_barrier().

And this is what I can't understand.

So let me repeat. Could you please describe a use-case which needs flush_rcuwork() with rcu_barrier() ?

Quoted 4 lines · Oleg Nesterov

Quoted 3 lines

flush_*work() guarantees to wait for the completion of the latest instance of the work item which was visible to the caller. We can't guarantee that w/o rcu_barrier().

Quoted 4 lines

And this is what I can't understand.

So let me repeat. Could you please describe a use-case which needs flush_rcuwork() with rcu_barrier() ?

So, if you skip that, flush_work() in itself won't wait for PENDING bit at all. It'll return right away if the work item is waiting for rcu grace period.

Still no use-case... But yes, I forgot this is needed for correctness.

OK, thanks for your patience. But fyi now I hate this interface even more, exactly because I was technically wrong in this discussion ;)