On Fri, Mar 06, 2020 at 11:22:07AM +0100, Vlastimil Babka wrote:
> On 3/2/20 8:36 PM, Minchan Kim wrote:
> > Bail out to prevent unnecessary CPU overhead if target process has
> > pending fatal signal during (MADV_COLD|MADV_PAGEOUT) operation.
> >
> > Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
>
> Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>
>
> Nit below:
>
> > ---
> > mm/madvise.c | 29 +++++++++++++++++++++--------
> > 1 file changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 349473fc6683..6543f2bfc3d8 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -36,6 +36,7 @@
> > struct madvise_walk_private {
> > struct mmu_gather *tlb;
> > bool pageout;
> > + struct task_struct *target_task;
> > };
> >
> > /*
> > @@ -316,6 +317,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> > if (fatal_signal_pending(current))
> > return -EINTR;
> >
> > + if (private->target_task &&
> > + fatal_signal_pending(private->target_task))
> > + return -EINTR;
>
> With madvise(2) private->target_task will be current, thus current will be
> tested twice. Not wrong, but maybe add a "private->target_task != current"
> condition?
It was in old series but removed because reviewer(IIRC, suren) wanted it.
I am not strong preference either way. Since you said it's nit and
considering other reviewer wanted to remove it, I will not change
further.
Thanks!
