lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 9 Apr 2021 08:43:17 -0700 From: Yang Shi <shy828301@...il.com> To: Oscar Salvador <osalvador@...e.de> Cc: Dave Hansen <dave.hansen@...ux.intel.com>, Linux MM <linux-mm@...ck.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Yang Shi <yang.shi@...ux.alibaba.com>, weixugc@...gle.com, Huang Ying <ying.huang@...el.com>, Dan Williams <dan.j.williams@...el.com>, David Hildenbrand <david@...hat.com> Subject: Re: [PATCH 04/10] mm/migrate: make migrate_pages() return nr_succeeded On Thu, Apr 8, 2021 at 10:06 PM Oscar Salvador <osalvador@...e.de> wrote: > > On Thu, Apr 08, 2021 at 01:40:33PM -0700, Yang Shi wrote: > > Thanks a lot for the example code. You didn't miss anything. At first > > glance, I thought your suggestion seemed neater. Actually I > > misunderstood what Dave said about "That could really have caused some > > interesting problems." with multiple calls to migrate_pages(). I was > > thinking about: > > > > unsigned long foo() > > { > > unsigned long *ret_succeeded; > > > > migrate_pages(..., ret_succeeded); > > > > migrate_pages(..., ret_succeeded); > > > > return *ret_succeeded; > > } > > But that would not be a problem as well. I mean I am not sure what is > foo() supposed to do. > I assume is supposed to return the *total* number of pages that were > migrated? > > Then could do something like: > > unsigned long foo() > { > unsigned long ret_succeeded; > unsigned long total_succeeded = 0; > > migrate_pages(..., &ret_succeeded); > total_succeeded += ret_succeeded; > > migrate_pages(..., &ret_succeeded); > total_succeeded += ret_succeeded; > > return *total_succeeded; > } > > But AFAICS, you would have to do that with Wei Xu's version and with > mine, no difference there. It is because nr_succeeded is reset for each migrate_pages() call. You could do "*ret_succeeded += nr_succeeded" if we want an accumulated counter, then you don't have to add total_succeeded. And since nr_succeeded is reset for each migrate_pages() call, so both vm counter and trace point are happy. > > IIUC, Dave's concern was that nr_succeeded was only set to 0 at the beginning > of the function, and never reset back, which means, we would carry the > sum of previous nr_succeeded instead of the nr_succeeded in that round. > That would be misleading for e.g: reclaim in case we were to call > migrate_pages() several times, as instead of a delta value, nr_succeeded > would accumulate. I think the most straightforward concern is the vm counter and trace point in migrate_pages(), if migrate_pages() is called multiple times we may see messed up counters if nr_succeeded is not reset properly. Of course both your and Wei's suggestion solve this problem. But if we have usecase which returns nr_succeeded and call migrate_pages() multiple times, I think we do want to return accumulated value IMHO. > > But that won't happen neither with Wei Xu's version nor with mine. > > -- > Oscar Salvador > SUSE L3
Powered by blists - more mailing lists