* [PATCH] delayacct: clear right task's flag after blkio completes
@ 2021-04-14 8:37 Yafang Shao
2021-04-19 7:00 ` Balbir Singh
0 siblings, 1 reply; 2+ messages in thread
From: Yafang Shao @ 2021-04-14 8:37 UTC (permalink / raw)
To: bsingharora, akpm
Cc: linux-mm, linux-kernel, Yafang Shao, Tejun Heo, Josh Snyder
When I was implementing a latency analyze tool by using task->delays and
other things, I found there's issue in delayacct. The issue is it should
clear the target's flag instead of current's in delayacct_blkio_end().
When I git blame delayacct, I found there're some similar issues we have
fixed in delayacct_blkio_end().
'Commit c96f5471ce7d ("delayacct: Account blkio completion on the correct task")'
fixed the issue that it should account blkio completion on the target
task instead of current.
'Commit b512719f771a ("delayacct: fix crash in delayacct_blkio_end() after delayacct init failure")'
fixed the issue that it should check target task's delays instead of
current task'. It seems that delayacct_blkio_{begin, end} are error prone.
So I introduce a new paratmeter - the target task 'p' into these helpers,
after that change, the callsite will specifilly set the right task, which
should make it less error prone.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Josh Snyder <joshs@netflix.com>
---
include/linux/delayacct.h | 20 ++++++++++----------
mm/memory.c | 8 ++++----
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/include/linux/delayacct.h b/include/linux/delayacct.h
index 2d3bdcccf5eb..21651f946751 100644
--- a/include/linux/delayacct.h
+++ b/include/linux/delayacct.h
@@ -82,16 +82,16 @@ static inline int delayacct_is_task_waiting_on_io(struct task_struct *p)
return 0;
}
-static inline void delayacct_set_flag(int flag)
+static inline void delayacct_set_flag(struct task_struct *p, int flag)
{
- if (current->delays)
- current->delays->flags |= flag;
+ if (p->delays)
+ p->delays->flags |= flag;
}
-static inline void delayacct_clear_flag(int flag)
+static inline void delayacct_clear_flag(struct task_struct *p, int flag)
{
- if (current->delays)
- current->delays->flags &= ~flag;
+ if (p->delays)
+ p->delays->flags &= ~flag;
}
static inline void delayacct_tsk_init(struct task_struct *tsk)
@@ -114,7 +114,7 @@ static inline void delayacct_tsk_free(struct task_struct *tsk)
static inline void delayacct_blkio_start(void)
{
- delayacct_set_flag(DELAYACCT_PF_BLKIO);
+ delayacct_set_flag(current, DELAYACCT_PF_BLKIO);
if (current->delays)
__delayacct_blkio_start();
}
@@ -123,7 +123,7 @@ static inline void delayacct_blkio_end(struct task_struct *p)
{
if (p->delays)
__delayacct_blkio_end(p);
- delayacct_clear_flag(DELAYACCT_PF_BLKIO);
+ delayacct_clear_flag(p, DELAYACCT_PF_BLKIO);
}
static inline int delayacct_add_tsk(struct taskstats *d,
@@ -166,9 +166,9 @@ static inline void delayacct_thrashing_end(void)
}
#else
-static inline void delayacct_set_flag(int flag)
+static inline void delayacct_set_flag(struct task_struct *p, int flag)
{}
-static inline void delayacct_clear_flag(int flag)
+static inline void delayacct_clear_flag(struct task_struct *p, int flag)
{}
static inline void delayacct_init(void)
{}
diff --git a/mm/memory.c b/mm/memory.c
index 550405fc3b5e..a013d32a6611 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3296,7 +3296,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
}
- delayacct_set_flag(DELAYACCT_PF_SWAPIN);
+ delayacct_set_flag(current, DELAYACCT_PF_SWAPIN);
page = lookup_swap_cache(entry, vma, vmf->address);
swapcache = page;
@@ -3347,7 +3347,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
vmf->address, &vmf->ptl);
if (likely(pte_same(*vmf->pte, vmf->orig_pte)))
ret = VM_FAULT_OOM;
- delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
+ delayacct_clear_flag(current, DELAYACCT_PF_SWAPIN);
goto unlock;
}
@@ -3361,13 +3361,13 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
* owner processes (which may be unknown at hwpoison time)
*/
ret = VM_FAULT_HWPOISON;
- delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
+ delayacct_clear_flag(current, DELAYACCT_PF_SWAPIN);
goto out_release;
}
locked = lock_page_or_retry(page, vma->vm_mm, vmf->flags);
- delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
+ delayacct_clear_flag(current, DELAYACCT_PF_SWAPIN);
if (!locked) {
ret |= VM_FAULT_RETRY;
goto out_release;
--
2.18.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] delayacct: clear right task's flag after blkio completes
2021-04-14 8:37 [PATCH] delayacct: clear right task's flag after blkio completes Yafang Shao
@ 2021-04-19 7:00 ` Balbir Singh
0 siblings, 0 replies; 2+ messages in thread
From: Balbir Singh @ 2021-04-19 7:00 UTC (permalink / raw)
To: Yafang Shao; +Cc: akpm, linux-mm, linux-kernel, Tejun Heo, Josh Snyder
On Wed, Apr 14, 2021 at 04:37:20PM +0800, Yafang Shao wrote:
> When I was implementing a latency analyze tool by using task->delays and
> other things, I found there's issue in delayacct. The issue is it should
> clear the target's flag instead of current's in delayacct_blkio_end().
>
> When I git blame delayacct, I found there're some similar issues we have
> fixed in delayacct_blkio_end().
> 'Commit c96f5471ce7d ("delayacct: Account blkio completion on the correct task")'
> fixed the issue that it should account blkio completion on the target
> task instead of current.
> 'Commit b512719f771a ("delayacct: fix crash in delayacct_blkio_end() after delayacct init failure")'
> fixed the issue that it should check target task's delays instead of
> current task'. It seems that delayacct_blkio_{begin, end} are error prone.
> So I introduce a new paratmeter - the target task 'p' into these helpers,
> after that change, the callsite will specifilly set the right task, which
> should make it less error prone.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Josh Snyder <joshs@netflix.com>
> ---
> include/linux/delayacct.h | 20 ++++++++++----------
> mm/memory.c | 8 ++++----
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/delayacct.h b/include/linux/delayacct.h
> index 2d3bdcccf5eb..21651f946751 100644
> --- a/include/linux/delayacct.h
> +++ b/include/linux/delayacct.h
> @@ -82,16 +82,16 @@ static inline int delayacct_is_task_waiting_on_io(struct task_struct *p)
> return 0;
> }
>
> -static inline void delayacct_set_flag(int flag)
> +static inline void delayacct_set_flag(struct task_struct *p, int flag)
> {
> - if (current->delays)
> - current->delays->flags |= flag;
> + if (p->delays)
> + p->delays->flags |= flag;
> }
>
> -static inline void delayacct_clear_flag(int flag)
> +static inline void delayacct_clear_flag(struct task_struct *p, int flag)
> {
> - if (current->delays)
> - current->delays->flags &= ~flag;
> + if (p->delays)
> + p->delays->flags &= ~flag;
> }
>
> static inline void delayacct_tsk_init(struct task_struct *tsk)
> @@ -114,7 +114,7 @@ static inline void delayacct_tsk_free(struct task_struct *tsk)
>
> static inline void delayacct_blkio_start(void)
> {
> - delayacct_set_flag(DELAYACCT_PF_BLKIO);
> + delayacct_set_flag(current, DELAYACCT_PF_BLKIO);
> if (current->delays)
> __delayacct_blkio_start();
> }
> @@ -123,7 +123,7 @@ static inline void delayacct_blkio_end(struct task_struct *p)
> {
> if (p->delays)
> __delayacct_blkio_end(p);
> - delayacct_clear_flag(DELAYACCT_PF_BLKIO);
> + delayacct_clear_flag(p, DELAYACCT_PF_BLKIO);
> }
>
> static inline int delayacct_add_tsk(struct taskstats *d,
> @@ -166,9 +166,9 @@ static inline void delayacct_thrashing_end(void)
> }
>
> #else
> -static inline void delayacct_set_flag(int flag)
> +static inline void delayacct_set_flag(struct task_struct *p, int flag)
> {}
> -static inline void delayacct_clear_flag(int flag)
> +static inline void delayacct_clear_flag(struct task_struct *p, int flag)
> {}
> static inline void delayacct_init(void)
> {}
> diff --git a/mm/memory.c b/mm/memory.c
> index 550405fc3b5e..a013d32a6611 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3296,7 +3296,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> }
>
>
> - delayacct_set_flag(DELAYACCT_PF_SWAPIN);
> + delayacct_set_flag(current, DELAYACCT_PF_SWAPIN);
> page = lookup_swap_cache(entry, vma, vmf->address);
> swapcache = page;
>
> @@ -3347,7 +3347,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> vmf->address, &vmf->ptl);
> if (likely(pte_same(*vmf->pte, vmf->orig_pte)))
> ret = VM_FAULT_OOM;
> - delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
> + delayacct_clear_flag(current, DELAYACCT_PF_SWAPIN);
> goto unlock;
> }
>
> @@ -3361,13 +3361,13 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> * owner processes (which may be unknown at hwpoison time)
> */
> ret = VM_FAULT_HWPOISON;
> - delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
> + delayacct_clear_flag(current, DELAYACCT_PF_SWAPIN);
> goto out_release;
> }
>
> locked = lock_page_or_retry(page, vma->vm_mm, vmf->flags);
>
> - delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
> + delayacct_clear_flag(current, DELAYACCT_PF_SWAPIN);
> if (!locked) {
> ret |= VM_FAULT_RETRY;
> goto out_release;
Acked-by: Balbir Singh <bsingharora@gmail.com>
The changes seem reasonable to me. I don't maintain a git tree, Andrew can we
please queue them up in your tree?
Balbir Singh.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-04-19 7:00 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14 8:37 [PATCH] delayacct: clear right task's flag after blkio completes Yafang Shao
2021-04-19 7:00 ` Balbir Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).