Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 19 Apr 2021 17:00:16 +1000
From:   Balbir Singh <bsingharora@gmail.com>
To:     Yafang Shao <laoar.shao@gmail.com>
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Tejun Heo <tj@kernel.org>,
        Josh Snyder <joshs@netflix.com>
Subject: Re: [PATCH] delayacct: clear right task's flag after blkio completes
Message-ID: <20210419070016.GB8178@balbir-desktop>
References: <20210414083720.24083-1-laoar.shao@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210414083720.24083-1-laoar.shao@gmail.com>
Precedence: bulk

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.