On Fri, Dec 18, 2015 at 11:00:41AM -0500, Johannes Weiner wrote:
> On Fri, Dec 18, 2015 at 06:32:02PM +0300, Vladimir Davydov wrote:
> > On Thu, Dec 17, 2015 at 03:02:17PM -0800, Andrew Morton wrote:
> > > On Tue, 15 Dec 2015 15:31:37 +0300 Vladimir Davydov <vdavydov@xxxxxxxxxxxxx> wrote:
> > > > @@ -859,14 +859,20 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> > > > if (prev && reclaim->generation != iter->generation)
> > > > goto out_unlock;
> > > >
> > > > - do {
> > > > + while (1) {
> > > > pos = READ_ONCE(iter->position);
> > > > + if (!pos || css_tryget(&pos->css))
> > > > + break;
> > > > /*
> > > > - * A racing update may change the position and
> > > > - * put the last reference, hence css_tryget(),
> > > > - * or retry to see the updated position.
> > > > + * css reference reached zero, so iter->position will
> > > > + * be cleared by ->css_released. However, we should not
> > > > + * rely on this happening soon, because ->css_released
> > > > + * is called from a work queue, and by busy-waiting we
> > > > + * might block it. So we clear iter->position right
> > > > + * away.
> > > > */
> > > > - } while (pos && !css_tryget(&pos->css));
> > > > + cmpxchg(&iter->position, pos, NULL);
> > > > + }
> > >
> > > It's peculiar to use cmpxchg() without actually checking that it did
> > > anything. Should we use xchg() here? And why aren't we using plain
> > > old "=", come to that?
> >
> > Well, it's obvious why we need the 'compare' part - the iter could have
> > been already advanced by a competing process, in which case we shouldn't
> > touch it, otherwise we would reclaim some cgroup twice during the same
> > reclaim generation. However, it's not that clear why it must be atomic.
> > Before this patch, atomicity was necessary to guarantee that we adjust
> > the reference counters correctly, but now we don't do it anymore. If a
> > competing process happens to update iter->position between the compare
> > and set steps, we might reclaim from the same cgroup twice at worst, and
> > this extremely unlikely to happen.
> >
> > So I think we can replace the atomic operation with a non-atomic one,
> > like the patch below does. Any objections?
>
> I don't think the race window is actually that small and reclaiming a
> group twice could cause sporadic latency issues in the victim group.
> Think about the group not just trimming caches but already swapping.
>
> The cmpxchg()s without checking the return values look odd without a
> comment, but that doesn't mean that they're wrong in this situation:
> advance the iterator from what we think is the current position, and
> don't if somebody beat us to that. That's what cmpxchg() does. So I'd
> rather we kept them here.
>
> > @@ -902,7 +903,15 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
> > }
> >
> > if (reclaim) {
> > - cmpxchg(&iter->position, pos, memcg);
> > + /*
> > + * The position could have already been updated by a competing
> > + * thread, so check that the value hasn't changed since we read
> > + * it. This operation doesn't need to be atomic, because a race
> > + * is extremely unlikely and in the worst case can only result
> > + * in the same cgroup reclaimed twice.
>
> But it would be good to add the first half of that comment to the
> cmpxchg to explain why we don't have to check the return value.
>
OK, got it, thanks. Here goes the incremental patch (it should also fix
the warning regarding unused cmpxchg returned value):
---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index fc25dc211eaf..908c075e04eb 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -864,7 +864,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
* might block it. So we clear iter->position right
* away.
*/
- cmpxchg(&iter->position, pos, NULL);
+ (void)cmpxchg(&iter->position, pos, NULL);
}
}
@@ -902,7 +902,12 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
}
if (reclaim) {
- cmpxchg(&iter->position, pos, memcg);
+ /*
+ * The position could have already been updated by a competing
+ * thread, so check that the value hasn't changed since we read
+ * it to avoid reclaiming from the same cgroup twice.
+ */
+ (void)cmpxchg(&iter->position, pos, memcg);
/*
* pairs with css_tryget when dereferencing iter->position
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>