[PATCH] proc/kpageflags: add KPF_WAITERS

[PATCH] proc/kpageflags: add KPF_WAITERS

[Konstantin Khlebnikov]

KPF_WAITERS indicates tasks are waiting for a page lock or writeback.
This might be false-positive, in this case next unlock will clear it.
This looks like worth information not only for kernel hacking.

In tool page-types in non-raw mode treat KPF_WAITERS without
KPF_LOCKED and KPF_WRITEBACK as false-positive and hide it.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/proc/page.c                         |    1 +
 include/uapi/linux/kernel-page-flags.h |    1 +
 tools/vm/page-types.c                  |    7 +++++++
 3 files changed, 9 insertions(+)

diff --git a/fs/proc/page.c b/fs/proc/page.c
index 1491918a33c3..b9312e1124af 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -159,6 +159,7 @@ u64 stable_page_flags(struct page *page)
 		u |= 1 << KPF_IDLE;
 
 	u |= kpf_copy_bit(k, KPF_LOCKED,	PG_locked);
+	u |= kpf_copy_bit(k, KPF_WAITERS,	PG_waiters);
 
 	u |= kpf_copy_bit(k, KPF_SLAB,		PG_slab);
 	if (PageTail(page) && PageSlab(compound_head(page)))
diff --git a/include/uapi/linux/kernel-page-flags.h b/include/uapi/linux/kernel-page-flags.h
index fa139841ec18..1118a028f2b3 100644
--- a/include/uapi/linux/kernel-page-flags.h
+++ b/include/uapi/linux/kernel-page-flags.h
@@ -35,6 +35,7 @@
 #define KPF_BALLOON		23
 #define KPF_ZERO_PAGE		24
 #define KPF_IDLE		25
+#define KPF_WAITERS		26
 
 
 #endif /* _UAPILINUX_KERNEL_PAGE_FLAGS_H */
diff --git a/tools/vm/page-types.c b/tools/vm/page-types.c
index a8783f48f77f..76d880b768b5 100644
--- a/tools/vm/page-types.c
+++ b/tools/vm/page-types.c
@@ -107,6 +107,7 @@
 
 static const char * const page_flag_names[] = {
 	[KPF_LOCKED]		= "L:locked",
+	[KPF_WAITERS]		= "Q:waiters",
 	[KPF_ERROR]		= "E:error",
 	[KPF_REFERENCED]	= "R:referenced",
 	[KPF_UPTODATE]		= "U:uptodate",
@@ -498,6 +499,12 @@ static uint64_t well_known_flags(uint64_t flags)
 	if ((flags & BITS_COMPOUND) && !(flags & BIT(HUGE)))
 		flags &= ~BITS_COMPOUND;
 
+	/* Treat WAITERS without LOCKED or WRITEBACK as false-postive */
+	if ((flags & (BIT(WAITERS) |
+		      BIT(LOCKED) |
+		      BIT(WRITEBACK))) == BIT(WAITERS))
+		flags &= ~BIT(WAITERS);
+
 	return flags;
 }
 

[PATCH] proc/kpageflags: add KPF_WAITERS

[Konstantin Khlebnikov]

KPF_WAITERS indicates tasks are waiting for a page lock or writeback.
This might be false-positive, in this case next unlock will clear it.
This looks like worth information not only for kernel hacking.

In tool page-types in non-raw mode treat KPF_WAITERS without
KPF_LOCKED and KPF_WRITEBACK as false-positive and hide it.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/proc/page.c                         |    1 +
 include/uapi/linux/kernel-page-flags.h |    1 +
 tools/vm/page-types.c                  |    7 +++++++
 3 files changed, 9 insertions(+)

diff --git a/fs/proc/page.c b/fs/proc/page.c
index 1491918a33c3..b9312e1124af 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -159,6 +159,7 @@ u64 stable_page_flags(struct page *page)
 		u |= 1 << KPF_IDLE;
 
 	u |= kpf_copy_bit(k, KPF_LOCKED,	PG_locked);
+	u |= kpf_copy_bit(k, KPF_WAITERS,	PG_waiters);
 
 	u |= kpf_copy_bit(k, KPF_SLAB,		PG_slab);
 	if (PageTail(page) && PageSlab(compound_head(page)))
diff --git a/include/uapi/linux/kernel-page-flags.h b/include/uapi/linux/kernel-page-flags.h
index fa139841ec18..1118a028f2b3 100644
--- a/include/uapi/linux/kernel-page-flags.h
+++ b/include/uapi/linux/kernel-page-flags.h
@@ -35,6 +35,7 @@
 #define KPF_BALLOON		23
 #define KPF_ZERO_PAGE		24
 #define KPF_IDLE		25
+#define KPF_WAITERS		26
 
 
 #endif /* _UAPILINUX_KERNEL_PAGE_FLAGS_H */
diff --git a/tools/vm/page-types.c b/tools/vm/page-types.c
index a8783f48f77f..76d880b768b5 100644
--- a/tools/vm/page-types.c
+++ b/tools/vm/page-types.c
@@ -107,6 +107,7 @@
 
 static const char * const page_flag_names[] = {
 	[KPF_LOCKED]		= "L:locked",
+	[KPF_WAITERS]		= "Q:waiters",
 	[KPF_ERROR]		= "E:error",
 	[KPF_REFERENCED]	= "R:referenced",
 	[KPF_UPTODATE]		= "U:uptodate",
@@ -498,6 +499,12 @@ static uint64_t well_known_flags(uint64_t flags)
 	if ((flags & BITS_COMPOUND) && !(flags & BIT(HUGE)))
 		flags &= ~BITS_COMPOUND;
 
+	/* Treat WAITERS without LOCKED or WRITEBACK as false-postive */
+	if ((flags & (BIT(WAITERS) |
+		      BIT(LOCKED) |
+		      BIT(WRITEBACK))) == BIT(WAITERS))
+		flags &= ~BIT(WAITERS);
+
 	return flags;
 }
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] proc/kpageflags: add KPF_WAITERS

[Andrew Morton]

On Sun, 11 Feb 2018 13:36:41 +0300 Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:

> KPF_WAITERS indicates tasks are waiting for a page lock or writeback.
> This might be false-positive, in this case next unlock will clear it.

Well, kpageflags is full of potential false-positives.  Or do you think
this flag is especially vulnerable?

In other words, under what circumstances will we have KPF_WAITERS set
when PG_locked and PG-writeback are clear?

> This looks like worth information not only for kernel hacking.

Why?  What are the use-cases, in detail?  How are we to justify this
modification?

> In tool page-types in non-raw mode treat KPF_WAITERS without
> KPF_LOCKED and KPF_WRITEBACK as false-positive and hide it.

>  fs/proc/page.c                         |    1 +
>  include/uapi/linux/kernel-page-flags.h |    1 +
>  tools/vm/page-types.c                  |    7 +++++++

Please update Documentation/vm/pagemap.txt.

Re: [PATCH] proc/kpageflags: add KPF_WAITERS

[Andrew Morton]

On Sun, 11 Feb 2018 13:36:41 +0300 Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:

> KPF_WAITERS indicates tasks are waiting for a page lock or writeback.
> This might be false-positive, in this case next unlock will clear it.

Well, kpageflags is full of potential false-positives.  Or do you think
this flag is especially vulnerable?

In other words, under what circumstances will we have KPF_WAITERS set
when PG_locked and PG-writeback are clear?

> This looks like worth information not only for kernel hacking.

Why?  What are the use-cases, in detail?  How are we to justify this
modification?

> In tool page-types in non-raw mode treat KPF_WAITERS without
> KPF_LOCKED and KPF_WRITEBACK as false-positive and hide it.

>  fs/proc/page.c                         |    1 +
>  include/uapi/linux/kernel-page-flags.h |    1 +
>  tools/vm/page-types.c                  |    7 +++++++

Please update Documentation/vm/pagemap.txt.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] proc/kpageflags: add KPF_WAITERS

[Konstantin Khlebnikov]

On 17.02.2018 02:57, Andrew Morton wrote:
> On Sun, 11 Feb 2018 13:36:41 +0300 Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:
> 
>> KPF_WAITERS indicates tasks are waiting for a page lock or writeback.
>> This might be false-positive, in this case next unlock will clear it.
> 
> Well, kpageflags is full of potential false-positives.  Or do you think
> this flag is especially vulnerable?
> 
> In other words, under what circumstances will we have KPF_WAITERS set
> when PG_locked and PG-writeback are clear?

Looks like lock_page() - unlock_page() shouldn't leave longstanding
false-positive: last unlock_page() must clear PG_waiters.

But I've seen them. Probably that was from  wait_on_page_writeback():
it test PG_writeback, set PG_waiters under queue lock unconditionally
and then test PG_writeback again before sleep - and might exit
without wakeup i.e. without clearing PG_waiters.

This could be fixed with extra check for in wait_on_page_bit_common()
under queue lock.

--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1087,6 +1087,10 @@ static inline int wait_on_page_bit_common(wait_queue_head_t *q,
                 spin_lock_irq(&q->lock);

                 if (likely(list_empty(&wait->entry))) {
+                       if (unlikely(!test_bit(bit_nr, &page->flags))) {
+                               spin_unlock_irq(&q->lock);
+                               goto try;
+                       }
                         __add_wait_queue_entry_tail(q, wait);
                         SetPageWaiters(page);
                 }
@@ -1098,7 +1102,7 @@ static inline int wait_on_page_bit_common(wait_queue_head_t *q,
                 if (likely(test_bit(bit_nr, &page->flags))) {
                         io_schedule();
                 }
-
+try:
                 if (lock) {
                         if (!test_and_set_bit_lock(bit_nr, &page->flags))
                                 break;

But this seems redundant.

> 
>> This looks like worth information not only for kernel hacking.
> 
> Why?  What are the use-cases, in detail?  How are we to justify this
> modification?

This bit tells which page or оffset in file is actually wanted
by somebody in the system. That's another way to track where major
faults or writeback blocks something. We don't have to record flow
of events - snapshot of page-flags will show where contention is.

> 
>> In tool page-types in non-raw mode treat KPF_WAITERS without
>> KPF_LOCKED and KPF_WRITEBACK as false-positive and hide it.
> 
>>   fs/proc/page.c                         |    1 +
>>   include/uapi/linux/kernel-page-flags.h |    1 +
>>   tools/vm/page-types.c                  |    7 +++++++
> 
> Please update Documentation/vm/pagemap.txt.
> 

ok

Re: [PATCH] proc/kpageflags: add KPF_WAITERS

[Konstantin Khlebnikov]

On 17.02.2018 02:57, Andrew Morton wrote:
> On Sun, 11 Feb 2018 13:36:41 +0300 Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:
> 
>> KPF_WAITERS indicates tasks are waiting for a page lock or writeback.
>> This might be false-positive, in this case next unlock will clear it.
> 
> Well, kpageflags is full of potential false-positives.  Or do you think
> this flag is especially vulnerable?
> 
> In other words, under what circumstances will we have KPF_WAITERS set
> when PG_locked and PG-writeback are clear?

Looks like lock_page() - unlock_page() shouldn't leave longstanding
false-positive: last unlock_page() must clear PG_waiters.

But I've seen them. Probably that was from  wait_on_page_writeback():
it test PG_writeback, set PG_waiters under queue lock unconditionally
and then test PG_writeback again before sleep - and might exit
without wakeup i.e. without clearing PG_waiters.

This could be fixed with extra check for in wait_on_page_bit_common()
under queue lock.

--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1087,6 +1087,10 @@ static inline int wait_on_page_bit_common(wait_queue_head_t *q,
                 spin_lock_irq(&q->lock);

                 if (likely(list_empty(&wait->entry))) {
+                       if (unlikely(!test_bit(bit_nr, &page->flags))) {
+                               spin_unlock_irq(&q->lock);
+                               goto try;
+                       }
                         __add_wait_queue_entry_tail(q, wait);
                         SetPageWaiters(page);
                 }
@@ -1098,7 +1102,7 @@ static inline int wait_on_page_bit_common(wait_queue_head_t *q,
                 if (likely(test_bit(bit_nr, &page->flags))) {
                         io_schedule();
                 }
-
+try:
                 if (lock) {
                         if (!test_and_set_bit_lock(bit_nr, &page->flags))
                                 break;

But this seems redundant.

> 
>> This looks like worth information not only for kernel hacking.
> 
> Why?  What are the use-cases, in detail?  How are we to justify this
> modification?

This bit tells which page or D 3/4 ffset in file is actually wanted
by somebody in the system. That's another way to track where major
faults or writeback blocks something. We don't have to record flow
of events - snapshot of page-flags will show where contention is.

> 
>> In tool page-types in non-raw mode treat KPF_WAITERS without
>> KPF_LOCKED and KPF_WRITEBACK as false-positive and hide it.
> 
>>   fs/proc/page.c                         |    1 +
>>   include/uapi/linux/kernel-page-flags.h |    1 +
>>   tools/vm/page-types.c                  |    7 +++++++
> 
> Please update Documentation/vm/pagemap.txt.
> 

ok

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] Documentation/vm/pagemap.txt: document bit WAITERS

[Konstantin Khlebnikov]

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 Documentation/vm/pagemap.txt |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/vm/pagemap.txt b/Documentation/vm/pagemap.txt
index eafcefa15261..eaa46771fa30 100644
--- a/Documentation/vm/pagemap.txt
+++ b/Documentation/vm/pagemap.txt
@@ -71,6 +71,7 @@ There are four components to pagemap:
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. WAITERS
 
  * /proc/kpagecgroup.  This file contains a 64-bit inode number of the
    memory cgroup each page is charged to, indexed by PFN. Only available when
@@ -127,6 +128,13 @@ Short descriptions to the page flags:
     stale in case the page was accessed via a PTE. To make sure the flag
     is up-to-date one has to read /sys/kernel/mm/page_idle/bitmap first.
 
+26. WAITERS
+    Indicates tasks are waiting when bits LOCKED or WRITEBACK will be cleared.
+    They might be blocked by undergoing IO or by contention on page lock.
+    Bit WAITERS might be false-positive, in this case next clear of LOCKED or
+    WRITEBACK will clear WAITERS too. I.e. without LOCKED and WRITEBACK it's
+    false-positive for sure.
+
     [IO related page flags]
  1. ERROR     IO error occurred
  3. UPTODATE  page has up-to-date data

[PATCH] Documentation/vm/pagemap.txt: document bit WAITERS

[Konstantin Khlebnikov]

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 Documentation/vm/pagemap.txt |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/vm/pagemap.txt b/Documentation/vm/pagemap.txt
index eafcefa15261..eaa46771fa30 100644
--- a/Documentation/vm/pagemap.txt
+++ b/Documentation/vm/pagemap.txt
@@ -71,6 +71,7 @@ There are four components to pagemap:
     23. BALLOON
     24. ZERO_PAGE
     25. IDLE
+    26. WAITERS
 
  * /proc/kpagecgroup.  This file contains a 64-bit inode number of the
    memory cgroup each page is charged to, indexed by PFN. Only available when
@@ -127,6 +128,13 @@ Short descriptions to the page flags:
     stale in case the page was accessed via a PTE. To make sure the flag
     is up-to-date one has to read /sys/kernel/mm/page_idle/bitmap first.
 
+26. WAITERS
+    Indicates tasks are waiting when bits LOCKED or WRITEBACK will be cleared.
+    They might be blocked by undergoing IO or by contention on page lock.
+    Bit WAITERS might be false-positive, in this case next clear of LOCKED or
+    WRITEBACK will clear WAITERS too. I.e. without LOCKED and WRITEBACK it's
+    false-positive for sure.
+
     [IO related page flags]
  1. ERROR     IO error occurred
  3. UPTODATE  page has up-to-date data

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] proc/kpageflags: add KPF_WAITERS

[Andrew Morton]

On Sat, 17 Feb 2018 11:14:10 +0300 Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:

> On 17.02.2018 02:57, Andrew Morton wrote:
> > On Sun, 11 Feb 2018 13:36:41 +0300 Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:
> > 
> >> KPF_WAITERS indicates tasks are waiting for a page lock or writeback.
> >> This might be false-positive, in this case next unlock will clear it.
> > 
> > Well, kpageflags is full of potential false-positives.  Or do you think
> > this flag is especially vulnerable?
> > 
> > In other words, under what circumstances will we have KPF_WAITERS set
> > when PG_locked and PG-writeback are clear?
> 
> Looks like lock_page() - unlock_page() shouldn't leave longstanding
> false-positive: last unlock_page() must clear PG_waiters.
> 
> But I've seen them. Probably that was from  wait_on_page_writeback():
> it test PG_writeback, set PG_waiters under queue lock unconditionally
> and then test PG_writeback again before sleep - and might exit
> without wakeup i.e. without clearing PG_waiters.
> 
> This could be fixed with extra check for in wait_on_page_bit_common()
> under queue lock.
>
> ...
>
> This bit tells which page or оffset in file is actually wanted
> by somebody in the system. That's another way to track where major
> faults or writeback blocks something. We don't have to record flow
> of events - snapshot of page-flags will show where contention is.
> 

Please send a v2 and let's get all this info into the changelog?

Re: [PATCH] proc/kpageflags: add KPF_WAITERS

[Andrew Morton]

On Sat, 17 Feb 2018 11:14:10 +0300 Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:

> On 17.02.2018 02:57, Andrew Morton wrote:
> > On Sun, 11 Feb 2018 13:36:41 +0300 Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:
> > 
> >> KPF_WAITERS indicates tasks are waiting for a page lock or writeback.
> >> This might be false-positive, in this case next unlock will clear it.
> > 
> > Well, kpageflags is full of potential false-positives.  Or do you think
> > this flag is especially vulnerable?
> > 
> > In other words, under what circumstances will we have KPF_WAITERS set
> > when PG_locked and PG-writeback are clear?
> 
> Looks like lock_page() - unlock_page() shouldn't leave longstanding
> false-positive: last unlock_page() must clear PG_waiters.
> 
> But I've seen them. Probably that was from  wait_on_page_writeback():
> it test PG_writeback, set PG_waiters under queue lock unconditionally
> and then test PG_writeback again before sleep - and might exit
> without wakeup i.e. without clearing PG_waiters.
> 
> This could be fixed with extra check for in wait_on_page_bit_common()
> under queue lock.
>
> ...
>
> This bit tells which page or D 3/4 ffset in file is actually wanted
> by somebody in the system. That's another way to track where major
> faults or writeback blocks something. We don't have to record flow
> of events - snapshot of page-flags will show where contention is.
> 

Please send a v2 and let's get all this info into the changelog?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>