| Message ID | 20210209174646.1310591-5-shy828301@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,01/12] mm: vmscan: use nid from shrink_control for tracepoint | expand |
On Tue, Feb 09, 2021 at 09:46:38AM -0800, Yang Shi wrote: > Both memcg_shrinker_map_size and shrinker_nr_max is maintained, but actually the > map size can be calculated via shrinker_nr_max, so it seems unnecessary to keep both. > Remove memcg_shrinker_map_size since shrinker_nr_max is also used by iterating the > bit map. > > Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com> > Signed-off-by: Yang Shi <shy828301@gmail.com> > --- > mm/vmscan.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index e4ddaaaeffe2..641077b09e5d 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -185,8 +185,10 @@ static LIST_HEAD(shrinker_list); > static DECLARE_RWSEM(shrinker_rwsem); > > #ifdef CONFIG_MEMCG > +static int shrinker_nr_max; > > -static int memcg_shrinker_map_size; > +#define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ > + (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) How about something like this? static inline int shrinker_map_size(int nr_items) { return DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long); } I think it look less cryptic. The rest of the patch looks good to me. Thanks!
On Tue, Feb 9, 2021 at 12:43 PM Roman Gushchin <guro@fb.com> wrote: > > On Tue, Feb 09, 2021 at 09:46:38AM -0800, Yang Shi wrote: > > Both memcg_shrinker_map_size and shrinker_nr_max is maintained, but actually the > > map size can be calculated via shrinker_nr_max, so it seems unnecessary to keep both. > > Remove memcg_shrinker_map_size since shrinker_nr_max is also used by iterating the > > bit map. > > > > Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com> > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > mm/vmscan.c | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index e4ddaaaeffe2..641077b09e5d 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -185,8 +185,10 @@ static LIST_HEAD(shrinker_list); > > static DECLARE_RWSEM(shrinker_rwsem); > > > > #ifdef CONFIG_MEMCG > > +static int shrinker_nr_max; > > > > -static int memcg_shrinker_map_size; > > +#define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ > > + (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) > > How about something like this? > > static inline int shrinker_map_size(int nr_items) > { > return DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long); > } > > I think it look less cryptic. OK, I don't have a strong opinion for either one (inline function or macro). If no one objects this I could do it in the new version. > > The rest of the patch looks good to me. > > Thanks!
On 2/9/21 9:43 PM, Roman Gushchin wrote: > On Tue, Feb 09, 2021 at 09:46:38AM -0800, Yang Shi wrote: >> Both memcg_shrinker_map_size and shrinker_nr_max is maintained, but actually the >> map size can be calculated via shrinker_nr_max, so it seems unnecessary to keep both. >> Remove memcg_shrinker_map_size since shrinker_nr_max is also used by iterating the >> bit map. >> >> Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com> >> Signed-off-by: Yang Shi <shy828301@gmail.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> >> --- >> mm/vmscan.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index e4ddaaaeffe2..641077b09e5d 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -185,8 +185,10 @@ static LIST_HEAD(shrinker_list); >> static DECLARE_RWSEM(shrinker_rwsem); >> >> #ifdef CONFIG_MEMCG >> +static int shrinker_nr_max; >> >> -static int memcg_shrinker_map_size; >> +#define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ >> + (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) > > How about something like this? > > static inline int shrinker_map_size(int nr_items) > { > return DIV_ROUND_UP(nr_items, BITS_PER_LONG) * sizeof(unsigned long); > } > > I think it look less cryptic. Yeah that looks nicer so I'm fine with that potential change. > The rest of the patch looks good to me. > > Thanks! >
diff --git a/mm/vmscan.c b/mm/vmscan.c index e4ddaaaeffe2..641077b09e5d 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -185,8 +185,10 @@ static LIST_HEAD(shrinker_list); static DECLARE_RWSEM(shrinker_rwsem); #ifdef CONFIG_MEMCG +static int shrinker_nr_max; -static int memcg_shrinker_map_size; +#define NR_MAX_TO_SHR_MAP_SIZE(nr_max) \ + (DIV_ROUND_UP(nr_max, BITS_PER_LONG) * sizeof(unsigned long)) static void free_shrinker_map_rcu(struct rcu_head *head) { @@ -247,7 +249,7 @@ int alloc_shrinker_maps(struct mem_cgroup *memcg) return 0; down_write(&shrinker_rwsem); - size = memcg_shrinker_map_size; + size = NR_MAX_TO_SHR_MAP_SIZE(shrinker_nr_max); for_each_node(nid) { map = kvzalloc_node(sizeof(*map) + size, GFP_KERNEL, nid); if (!map) { @@ -265,12 +267,13 @@ int alloc_shrinker_maps(struct mem_cgroup *memcg) static int expand_shrinker_maps(int new_id) { int size, old_size, ret = 0; + int new_nr_max = new_id + 1; struct mem_cgroup *memcg; - size = DIV_ROUND_UP(new_id + 1, BITS_PER_LONG) * sizeof(unsigned long); - old_size = memcg_shrinker_map_size; + size = NR_MAX_TO_SHR_MAP_SIZE(new_nr_max); + old_size = NR_MAX_TO_SHR_MAP_SIZE(shrinker_nr_max); if (size <= old_size) - return 0; + goto out; if (!root_mem_cgroup) goto out; @@ -287,7 +290,7 @@ static int expand_shrinker_maps(int new_id) } while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)) != NULL); out: if (!ret) - memcg_shrinker_map_size = size; + shrinker_nr_max = new_nr_max; return ret; } @@ -320,7 +323,6 @@ void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id) #define SHRINKER_REGISTERING ((struct shrinker *)~0UL) static DEFINE_IDR(shrinker_idr); -static int shrinker_nr_max; static int prealloc_memcg_shrinker(struct shrinker *shrinker) { @@ -337,8 +339,6 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker) idr_remove(&shrinker_idr, id); goto unlock; } - - shrinker_nr_max = id + 1; } shrinker->id = id; ret = 0;