| Message ID | 155790967469.1319.14744588086607025680.stgit@buzz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] proc: use down_read_killable for /proc/pid/maps | expand |
On Wed 15-05-19 11:41:14, Konstantin Khlebnikov wrote: > Ditto. Proper changelog or simply squash those patches into a single patch if you do not feel like copy&paste is fun > > Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> > --- > fs/proc/task_mmu.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index 2bf210229daf..781879a91e3b 100644 > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -832,7 +832,10 @@ static int show_smaps_rollup(struct seq_file *m, void *v) > > memset(&mss, 0, sizeof(mss)); > > - down_read(&mm->mmap_sem); > + ret = down_read_killable(&mm->mmap_sem); > + if (ret) > + goto out_put_mm; Why not ret = -EINTR. The seq_file code seems to be handling all errors AFAICS. > + > hold_task_mempolicy(priv); > > for (vma = priv->mm->mmap; vma; vma = vma->vm_next) { > @@ -849,8 +852,9 @@ static int show_smaps_rollup(struct seq_file *m, void *v) > > release_task_mempolicy(priv); > up_read(&mm->mmap_sem); > - mmput(mm); > > +out_put_mm: > + mmput(mm); > out_put_task: > put_task_struct(priv->task); > priv->task = NULL;
On 17.05.2019 15:45, Michal Hocko wrote: > On Wed 15-05-19 11:41:14, Konstantin Khlebnikov wrote: >> Ditto. > > Proper changelog or simply squash those patches into a single patch if > you do not feel like copy&paste is fun > >> >> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> >> --- >> fs/proc/task_mmu.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c >> index 2bf210229daf..781879a91e3b 100644 >> --- a/fs/proc/task_mmu.c >> +++ b/fs/proc/task_mmu.c >> @@ -832,7 +832,10 @@ static int show_smaps_rollup(struct seq_file *m, void *v) >> >> memset(&mss, 0, sizeof(mss)); >> >> - down_read(&mm->mmap_sem); >> + ret = down_read_killable(&mm->mmap_sem); >> + if (ret) >> + goto out_put_mm; > > Why not ret = -EINTR. The seq_file code seems to be handling all errors > AFAICS. > I've missed your comment. Sorry. down_read_killable returns 0 for success and exactly -EINTR for failure. >> + >> hold_task_mempolicy(priv); >> >> for (vma = priv->mm->mmap; vma; vma = vma->vm_next) { >> @@ -849,8 +852,9 @@ static int show_smaps_rollup(struct seq_file *m, void *v) >> >> release_task_mempolicy(priv); >> up_read(&mm->mmap_sem); >> - mmput(mm); >> >> +out_put_mm: >> + mmput(mm); >> out_put_task: >> put_task_struct(priv->task); >> priv->task = NULL; >
On Sun 09-06-19 12:07:36, Konstantin Khlebnikov wrote: [...] > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > > index 2bf210229daf..781879a91e3b 100644 > > > --- a/fs/proc/task_mmu.c > > > +++ b/fs/proc/task_mmu.c > > > @@ -832,7 +832,10 @@ static int show_smaps_rollup(struct seq_file *m, void *v) > > > memset(&mss, 0, sizeof(mss)); > > > - down_read(&mm->mmap_sem); > > > + ret = down_read_killable(&mm->mmap_sem); > > > + if (ret) > > > + goto out_put_mm; > > > > Why not ret = -EINTR. The seq_file code seems to be handling all errors > > AFAICS. > > > > I've missed your comment. Sorry. > > down_read_killable returns 0 for success and exactly -EINTR for failure. You are right of course. I must have misread the code at the time. Sorry about that.
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 2bf210229daf..781879a91e3b 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -832,7 +832,10 @@ static int show_smaps_rollup(struct seq_file *m, void *v) memset(&mss, 0, sizeof(mss)); - down_read(&mm->mmap_sem); + ret = down_read_killable(&mm->mmap_sem); + if (ret) + goto out_put_mm; + hold_task_mempolicy(priv); for (vma = priv->mm->mmap; vma; vma = vma->vm_next) { @@ -849,8 +852,9 @@ static int show_smaps_rollup(struct seq_file *m, void *v) release_task_mempolicy(priv); up_read(&mm->mmap_sem); - mmput(mm); +out_put_mm: + mmput(mm); out_put_task: put_task_struct(priv->task); priv->task = NULL;
Ditto. Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> --- fs/proc/task_mmu.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)