On Wed, Oct 25, 2017 at 03:49:21PM -0700, Greg Thelen wrote: > Johannes Weiner <hannes@xxxxxxxxxxx> wrote: > > > On Wed, Oct 25, 2017 at 09:00:57PM +0200, Michal Hocko wrote: > >> On Wed 25-10-17 14:11:06, Johannes Weiner wrote: > >> > "Safe" is a vague term, and it doesn't make much sense to me in this > >> > situation. The OOM behavior should be predictable and consistent. > >> > > >> > Yes, global might in the rarest cases also return -ENOMEM. Maybe. We > >> > don't have to do that in memcg because we're not physically limited. > >> > >> OK, so here seems to be the biggest disconnect. Being physically or > >> artificially constrained shouldn't make much difference IMHO. In both > >> cases the resource is simply limited for the consumer. And once all the > >> attempts to fit within the limit fail then the request for the resource > >> has to fail. > > > > It's a huge difference. In the global case, we have to make trade-offs > > to not deadlock the kernel. In the memcg case, we have to make a trade > > off between desirable OOM behavior and desirable meaning of memory.max. > > > > If we can borrow a resource temporarily from the ether to resolve the > > OOM situation, I don't see why we shouldn't. We're only briefly > > ignoring the limit to make sure the allocating task isn't preventing > > the OOM victim from exiting or the OOM reaper from reaping. It's more > > of an implementation detail than interface. > > > > The only scenario you brought up where this might be the permanent > > overrun is the single, oom-disabled task. And I explained why that is > > a silly argument, why that's the least problematic consequence of > > oom-disabling, and why it probably shouldn't even be configurable. > > > > The idea that memory.max must never be breached is an extreme and > > narrow view. As Greg points out, there are allocations we do not even > > track. There are other scenarios that force allocations. They may > > violate the limit on paper, but they're not notably weakening the goal > > of memory.max - isolating workloads from each other. > > > > Let's look at it this way. > > > > There are two deadlock relationships the OOM killer needs to solve > > between the triggerer and the potential OOM victim: > > > > #1 Memory. The triggerer needs memory that the victim has, > > but the victim needs some additional memory to release it. > > > > #2 Locks. The triggerer needs memory that the victim has, but > > the victim needs a lock the triggerer holds to release it. > > > > We have no qualms letting the victim temporarily (until the victim's > > exit) ignore memory.max to resolve the memory deadlock #1. > > > > I don't understand why it's such a stretch to let the triggerer > > temporarily (until the victim's exit) ignore memory.max to resolve the > > locks deadlock #2. [1] > > > > We need both for the OOM killer to function correctly. > > > > We've solved #1 both for memcg and globally. But we haven't solved #2. > > Global can still deadlock, and memcg copped out and returns -ENOMEM. > > > > Adding speculative OOM killing before the -ENOMEM makes things more > > muddy and unpredictable. It doesn't actually solve deadlock #2. > > > > [1] And arguably that's what we should be doing in the global case > > too: give the triggerer access to reserves. If you recall this > > thread here: https://patchwork.kernel.org/patch/6088511/ > > > >> > > So the only change I am really proposing is to keep retrying as long > >> > > as the oom killer makes a forward progress and ENOMEM otherwise. > >> > > >> > That's the behavior change I'm against. > >> > >> So just to make it clear you would be OK with the retry on successful > >> OOM killer invocation and force charge on oom failure, right? > > > > Yeah, that sounds reasonable to me. > > Assuming we're talking about retrying within try_charge(), then there's > a detail to iron out... > > If there is a pending oom victim blocked on a lock held by try_charge() caller > (the "#2 Locks" case), then I think repeated calls to out_of_memory() will > return true until the victim either gets MMF_OOM_SKIP or disappears. So a force > charge fallback might be a needed even with oom killer successful invocations. > Or we'll need to teach out_of_memory() to return three values (e.g. NO_VICTIM, > NEW_VICTIM, PENDING_VICTIM) and try_charge() can loop on NEW_VICTIM. True. I was assuming we'd retry MEM_CGROUP_RECLAIM_RETRIES times at a maximum, even if the OOM killer indicates a kill has been issued. What you propose makes sense too.
