On Tue, Aug 6, 2019 at 11:59 PM Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> wrote: > > On Tue, Aug 06, 2019 at 07:35:29PM +0800, Yafang Shao wrote: > > On Tue, Aug 6, 2019 at 7:15 PM Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx> wrote: > > > > > > On Tue, Aug 06, 2019 at 05:32:54PM +0800, Yafang Shao wrote: > > > > On Tue, Aug 6, 2019 at 5:25 PM Michal Hocko <mhocko@xxxxxxxxxx> wrote: > > > > > > > > > > On Tue 06-08-19 17:15:05, Yafang Shao wrote: > > > > > > On Tue, Aug 6, 2019 at 5:05 PM Michal Hocko <mhocko@xxxxxxxxxx> wrote: > > > > > [...] > > > > > > > > As you said, the direct reclaim path set it to 1, but the > > > > > > > > __node_reclaim() forgot to process may_shrink_slab. > > > > > > > > > > > > > > OK, I am blind obviously. Sorry about that. Anyway, why cannot we simply > > > > > > > get back to the original behavior by setting may_shrink_slab in that > > > > > > > path as well? > > > > > > > > > > > > You mean do it as the commit 0ff38490c836 did before ? > > > > > > I haven't check in which commit the shrink_slab() is removed from > > > > > > > > > > What I've had in mind was essentially this: > > > > > > > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > > > > index 7889f583ced9..8011288a80e2 100644 > > > > > --- a/mm/vmscan.c > > > > > +++ b/mm/vmscan.c > > > > > @@ -4088,6 +4093,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in > > > > > .may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP), > > > > > .may_swap = 1, > > > > > .reclaim_idx = gfp_zone(gfp_mask), > > > > > + .may_shrinkslab = 1; > > > > > }; > > > > > > > > > > trace_mm_vmscan_node_reclaim_begin(pgdat->node_id, order, > > > > > > > > > > shrink_node path already does shrink slab when the flag allows that. In > > > > > other words get us back to before 1c30844d2dfe because that has clearly > > > > > changed the long term node reclaim behavior just recently. > > > > > -- > > > > > > > > If we do it like this, then vm.min_slab_ratio will not take effect if > > > > there're enough relcaimable page cache. > > > > Seems there're bugs in the original behavior as well. > > > > > > > > > > Typically that would be done as a separate patch with a standalone > > > justification for it. The first patch should simply restore expected > > > behaviour with a Fixes: tag noting that the change in behaviour was > > > unintentional. > > > > > > > Sure, I will do it. > > Do you plan to send the second patch? If not I think we should at least update > the documentation for the admittedly obscure vm.min_slab_ratio to reflect its > effect on node reclaim, which is currently none. I don't have a explicit plan when to post the second patch because I'm not sure when it will be ready. If your workload depends on vm.min_slab_ratio, you could post a fix for it if you would like to. I will appreciate it. I don't think it is a good idea to document it, because this is not a limitation, while it is really a issue. Thanks Yafang
