| Message ID | 20220706155848.4939-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: Minor fixes for non-preallocated memory | expand |
On Wed, Jul 6, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > if we allocate too much GFP_ATOMIC memory. For example, when we set the > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > easily break the memcg limit by force charge. So it is very dangerous to > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > too much memory. > > We introduced BPF_F_NO_PREALLOC is because full map pre-allocation is > too memory expensive for some cases. That means removing __GFP_HIGH > doesn't break the rule of BPF_F_NO_PREALLOC, but has the same goal with > it-avoiding issues caused by too much memory. So let's remove it. > > The force charge of GFP_ATOMIC was introduced in > commit 869712fd3de5 ("mm: memcontrol: fix network errors from failing > __GFP_ATOMIC charges") by checking __GFP_ATOMIC, then got improved in > commit 1461e8c2b6af ("memcg: unify force charging conditions") by > checking __GFP_HIGH (that is no problem because both __GFP_HIGH and > __GFP_ATOMIC are set in GFP_AOMIC). So, if we want to fix it in memcg, > we have to carefully verify all the callsites. Now that we can fix it in > BPF, we'd better not modify the memcg code. > > This fix can also apply to other run-time allocations, for example, the > allocation in lpm trie, local storage and devmap. So let fix it > consistently over the bpf code > > __GFP_KSWAPD_RECLAIM doesn't cooperate well with memcg pressure neither > currently. But the memcg code can be improved to make > __GFP_KSWAPD_RECLAIM work well under memcg pressure if desired. Could you elaborate ? > It also fixes a typo in the comment. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev> Roman, do you agree with this change ?
On Wed, Jul 06, 2022 at 09:47:32AM -0700, Alexei Starovoitov wrote: > On Wed, Jul 6, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > easily break the memcg limit by force charge. So it is very dangerous to > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > too much memory. > > > > We introduced BPF_F_NO_PREALLOC is because full map pre-allocation is > > too memory expensive for some cases. That means removing __GFP_HIGH > > doesn't break the rule of BPF_F_NO_PREALLOC, but has the same goal with > > it-avoiding issues caused by too much memory. So let's remove it. > > > > The force charge of GFP_ATOMIC was introduced in > > commit 869712fd3de5 ("mm: memcontrol: fix network errors from failing > > __GFP_ATOMIC charges") by checking __GFP_ATOMIC, then got improved in > > commit 1461e8c2b6af ("memcg: unify force charging conditions") by > > checking __GFP_HIGH (that is no problem because both __GFP_HIGH and > > __GFP_ATOMIC are set in GFP_AOMIC). So, if we want to fix it in memcg, > > we have to carefully verify all the callsites. Now that we can fix it in > > BPF, we'd better not modify the memcg code. > > > > This fix can also apply to other run-time allocations, for example, the > > allocation in lpm trie, local storage and devmap. So let fix it > > consistently over the bpf code > > > > __GFP_KSWAPD_RECLAIM doesn't cooperate well with memcg pressure neither > > currently. But the memcg code can be improved to make > > __GFP_KSWAPD_RECLAIM work well under memcg pressure if desired. > > Could you elaborate ? > > > It also fixes a typo in the comment. > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev> > > Roman, do you agree with this change ? Yes, removing __GFP_HIGH makes sense to me. I can imagine we might want it for *some* bpf allocations, but applying it unconditionally looks wrong. Thanks!
On Wed, Jul 6, 2022 at 12:09 PM Roman Gushchin <roman.gushchin@linux.dev> wrote: > > On Wed, Jul 06, 2022 at 09:47:32AM -0700, Alexei Starovoitov wrote: > > On Wed, Jul 6, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > > easily break the memcg limit by force charge. So it is very dangerous to > > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > > too much memory. > > > > > > We introduced BPF_F_NO_PREALLOC is because full map pre-allocation is > > > too memory expensive for some cases. That means removing __GFP_HIGH > > > doesn't break the rule of BPF_F_NO_PREALLOC, but has the same goal with > > > it-avoiding issues caused by too much memory. So let's remove it. > > > > > > The force charge of GFP_ATOMIC was introduced in > > > commit 869712fd3de5 ("mm: memcontrol: fix network errors from failing > > > __GFP_ATOMIC charges") by checking __GFP_ATOMIC, then got improved in > > > commit 1461e8c2b6af ("memcg: unify force charging conditions") by > > > checking __GFP_HIGH (that is no problem because both __GFP_HIGH and > > > __GFP_ATOMIC are set in GFP_AOMIC). So, if we want to fix it in memcg, > > > we have to carefully verify all the callsites. Now that we can fix it in > > > BPF, we'd better not modify the memcg code. > > > > > > This fix can also apply to other run-time allocations, for example, the > > > allocation in lpm trie, local storage and devmap. So let fix it > > > consistently over the bpf code > > > > > > __GFP_KSWAPD_RECLAIM doesn't cooperate well with memcg pressure neither > > > currently. But the memcg code can be improved to make > > > __GFP_KSWAPD_RECLAIM work well under memcg pressure if desired. > > > > Could you elaborate ? > > > > > It also fixes a typo in the comment. > > > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > > Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev> > > > > Roman, do you agree with this change ? > > Yes, removing __GFP_HIGH makes sense to me. I can imagine we might want > it for *some* bpf allocations, but applying it unconditionally looks wrong. Yeah. It's a difficult trade-off to make without having the data to decide whether removing __GFP_HIGH can cause issues or not, but do you agree that __GFP_HIGH doesn't cooperate well with memcg ? If so it's a bug on memcg side, right? but we should probably apply this band-aid on bpf side to fix the bleeding. Later we can add a knob to allow __GFP_HIGH usage on demand from bpf prog.
On Wed, Jul 06, 2022 at 03:11:46PM -0700, Alexei Starovoitov wrote: > On Wed, Jul 6, 2022 at 12:09 PM Roman Gushchin <roman.gushchin@linux.dev> wrote: > > > > On Wed, Jul 06, 2022 at 09:47:32AM -0700, Alexei Starovoitov wrote: > > > On Wed, Jul 6, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > > > easily break the memcg limit by force charge. So it is very dangerous to > > > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > > > too much memory. > > > > > > > > We introduced BPF_F_NO_PREALLOC is because full map pre-allocation is > > > > too memory expensive for some cases. That means removing __GFP_HIGH > > > > doesn't break the rule of BPF_F_NO_PREALLOC, but has the same goal with > > > > it-avoiding issues caused by too much memory. So let's remove it. > > > > > > > > The force charge of GFP_ATOMIC was introduced in > > > > commit 869712fd3de5 ("mm: memcontrol: fix network errors from failing > > > > __GFP_ATOMIC charges") by checking __GFP_ATOMIC, then got improved in > > > > commit 1461e8c2b6af ("memcg: unify force charging conditions") by > > > > checking __GFP_HIGH (that is no problem because both __GFP_HIGH and > > > > __GFP_ATOMIC are set in GFP_AOMIC). So, if we want to fix it in memcg, > > > > we have to carefully verify all the callsites. Now that we can fix it in > > > > BPF, we'd better not modify the memcg code. > > > > > > > > This fix can also apply to other run-time allocations, for example, the > > > > allocation in lpm trie, local storage and devmap. So let fix it > > > > consistently over the bpf code > > > > > > > > __GFP_KSWAPD_RECLAIM doesn't cooperate well with memcg pressure neither > > > > currently. But the memcg code can be improved to make > > > > __GFP_KSWAPD_RECLAIM work well under memcg pressure if desired. > > > > > > Could you elaborate ? > > > > > > > It also fixes a typo in the comment. > > > > > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > > > Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev> > > > > > > Roman, do you agree with this change ? > > > > Yes, removing __GFP_HIGH makes sense to me. I can imagine we might want > > it for *some* bpf allocations, but applying it unconditionally looks wrong. > > Yeah. It's a difficult trade-off to make without having the data > to decide whether removing __GFP_HIGH can cause issues or not, Yeah, the change looks reasonable, but it's hard to say without giving it a good testing in (something close to) a production environment. > but do you agree that __GFP_HIGH doesn't cooperate well with memcg ? > If so it's a bug on memcg side, right? No. Historically we allowed high-prio allocations to exceed the memcg limit because otherwise there were too many stability and performance issues. It's not a memcg bug, it's a way to avoid exposing ENOMEM handling bugs all over the kernel code. Without memory cgroups GFP_ATOMIC allocations rarely fail and a lot of code paths in the kernel are not really ready for it (at least it was the case several years ago, maybe things are better now). But it was usually thought in the context of small(ish) allocations which do not change the global memory usage picture. Subsequent "normal" allocations are triggering reclaim/OOM, so from a user's POV the limit works as expected. But with the ownership model and size of bpf maps it's a different story: if a bpf map belongs to an abandoned cgroup, it might consume a lot of memory and there will be no "normal" allocations. So cgroup memory limit will be never applied. It's a valid issue, I agree with Yafang here. > but we should probably > apply this band-aid on bpf side to fix the bleeding. > Later we can add a knob to allow __GFP_HIGH usage on demand from > bpf prog. Yes, it sounds like a good idea. I have to think what's the best approach here, it's not obvious for me. Thanks!
On Wed, Jul 6, 2022 at 3:54 PM Roman Gushchin <roman.gushchin@linux.dev> wrote: > > On Wed, Jul 06, 2022 at 03:11:46PM -0700, Alexei Starovoitov wrote: > > On Wed, Jul 6, 2022 at 12:09 PM Roman Gushchin <roman.gushchin@linux.dev> wrote: > > > > > > On Wed, Jul 06, 2022 at 09:47:32AM -0700, Alexei Starovoitov wrote: > > > > On Wed, Jul 6, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > > > > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > > > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > > > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > > > > easily break the memcg limit by force charge. So it is very dangerous to > > > > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > > > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > > > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > > > > too much memory. > > > > > > > > > > We introduced BPF_F_NO_PREALLOC is because full map pre-allocation is > > > > > too memory expensive for some cases. That means removing __GFP_HIGH > > > > > doesn't break the rule of BPF_F_NO_PREALLOC, but has the same goal with > > > > > it-avoiding issues caused by too much memory. So let's remove it. > > > > > > > > > > The force charge of GFP_ATOMIC was introduced in > > > > > commit 869712fd3de5 ("mm: memcontrol: fix network errors from failing > > > > > __GFP_ATOMIC charges") by checking __GFP_ATOMIC, then got improved in > > > > > commit 1461e8c2b6af ("memcg: unify force charging conditions") by > > > > > checking __GFP_HIGH (that is no problem because both __GFP_HIGH and > > > > > __GFP_ATOMIC are set in GFP_AOMIC). So, if we want to fix it in memcg, > > > > > we have to carefully verify all the callsites. Now that we can fix it in > > > > > BPF, we'd better not modify the memcg code. > > > > > > > > > > This fix can also apply to other run-time allocations, for example, the > > > > > allocation in lpm trie, local storage and devmap. So let fix it > > > > > consistently over the bpf code > > > > > > > > > > __GFP_KSWAPD_RECLAIM doesn't cooperate well with memcg pressure neither > > > > > currently. But the memcg code can be improved to make > > > > > __GFP_KSWAPD_RECLAIM work well under memcg pressure if desired. > > > > > > > > Could you elaborate ? > > > > > > > > > It also fixes a typo in the comment. > > > > > > > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > > > > Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev> > > > > > > > > Roman, do you agree with this change ? > > > > > > Yes, removing __GFP_HIGH makes sense to me. I can imagine we might want > > > it for *some* bpf allocations, but applying it unconditionally looks wrong. > > > > Yeah. It's a difficult trade-off to make without having the data > > to decide whether removing __GFP_HIGH can cause issues or not, > > Yeah, the change looks reasonable, but it's hard to say without giving > it a good testing in (something close to) a production environment. > > > but do you agree that __GFP_HIGH doesn't cooperate well with memcg ? > > If so it's a bug on memcg side, right? > > No. Historically we allowed high-prio allocations to exceed the memcg limit > because otherwise there were too many stability and performance issues. > It's not a memcg bug, it's a way to avoid exposing ENOMEM handling bugs all over > the kernel code. Without memory cgroups GFP_ATOMIC allocations rarely fail > and a lot of code paths in the kernel are not really ready for it (at least > it was the case several years ago, maybe things are better now). > > But it was usually thought in the context of small(ish) allocations which do not > change the global memory usage picture. Subsequent "normal" allocations are > triggering reclaim/OOM, so from a user's POV the limit works as expected. > > But with the ownership model and size of bpf maps it's a different story: > if a bpf map belongs to an abandoned cgroup, it might consume a lot of memory > and there will be no "normal" allocations. So cgroup memory limit will be never > applied. It's a valid issue, I agree with Yafang here. Understood. > > but we should probably > > apply this band-aid on bpf side to fix the bleeding. > > Later we can add a knob to allow __GFP_HIGH usage on demand from > > bpf prog. > > Yes, it sounds like a good idea. I have to think what's the best approach > here, it's not obvious for me. Ok. Applied this patch for now.
On Wed, Jul 06, 2022 at 03:58:47PM +0000, Yafang Shao wrote: > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > if we allocate too much GFP_ATOMIC memory. For example, when we set the > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > easily break the memcg limit by force charge. So it is very dangerous to > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > too much memory. Please use GFP_NOWAIT instead of (__GFP_ATOMIC | __GFP_KSWAPD_RECLAIM). There is already a plan to completely remove __GFP_ATOMIC and mm-tree already have a patch for that. > > We introduced BPF_F_NO_PREALLOC is because full map pre-allocation is > too memory expensive for some cases. That means removing __GFP_HIGH > doesn't break the rule of BPF_F_NO_PREALLOC, but has the same goal with > it-avoiding issues caused by too much memory. So let's remove it. > > The force charge of GFP_ATOMIC was introduced in > commit 869712fd3de5 ("mm: memcontrol: fix network errors from failing > __GFP_ATOMIC charges") by checking __GFP_ATOMIC, then got improved in > commit 1461e8c2b6af ("memcg: unify force charging conditions") by > checking __GFP_HIGH (that is no problem because both __GFP_HIGH and > __GFP_ATOMIC are set in GFP_AOMIC). So, if we want to fix it in memcg, > we have to carefully verify all the callsites. Now that we can fix it in > BPF, we'd better not modify the memcg code. > > This fix can also apply to other run-time allocations, for example, the > allocation in lpm trie, local storage and devmap. So let fix it > consistently over the bpf code > > __GFP_KSWAPD_RECLAIM doesn't cooperate well with memcg pressure neither > currently. But the memcg code can be improved to make > __GFP_KSWAPD_RECLAIM work well under memcg pressure if desired. > IMO there is no need to give all this detail and background on GFP_ATOMIC and __GFP_KSWAPD_RECLAIM. Just say kernel allows GFP_ATOMIC allocations to exceed memcg limits which we don't want in this case. So, replace with GFP_NOWAIT which obey memcg limits. Both of these flags tell kernel that the caller can not sleep.
On Wed, Jul 6, 2022 at 5:07 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Wed, Jul 06, 2022 at 03:58:47PM +0000, Yafang Shao wrote: > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > easily break the memcg limit by force charge. So it is very dangerous to > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > too much memory. > > Please use GFP_NOWAIT instead of (__GFP_ATOMIC | __GFP_KSWAPD_RECLAIM). > There is already a plan to completely remove __GFP_ATOMIC and mm-tree > already have a patch for that. hmm. ok. reverted.
On Thu, Jul 07, 2022 at 12:07:21AM +0000, Shakeel Butt wrote: > On Wed, Jul 06, 2022 at 03:58:47PM +0000, Yafang Shao wrote: > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > easily break the memcg limit by force charge. So it is very dangerous to > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > too much memory. > > Please use GFP_NOWAIT instead of (__GFP_ATOMIC | __GFP_KSWAPD_RECLAIM). > There is already a plan to completely remove __GFP_ATOMIC and mm-tree > already have a patch for that. Oh, I didn't know this, thanks for heads up! I agree that GFP_NOWAIT is the best choice then. Btw, we probably shouldn't even add GFP_NOWAIT if the allocation is performed from the bpf syscall context. Why would we fail to pre-allocate a map if we can easily go into the reclaim? But probably better to leave it for a separate change. Thanks!
On Wed, Jul 6, 2022 at 5:25 PM Roman Gushchin <roman.gushchin@linux.dev> wrote: > > On Thu, Jul 07, 2022 at 12:07:21AM +0000, Shakeel Butt wrote: > > On Wed, Jul 06, 2022 at 03:58:47PM +0000, Yafang Shao wrote: > > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > > easily break the memcg limit by force charge. So it is very dangerous to > > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > > too much memory. > > > > Please use GFP_NOWAIT instead of (__GFP_ATOMIC | __GFP_KSWAPD_RECLAIM). > > There is already a plan to completely remove __GFP_ATOMIC and mm-tree > > already have a patch for that. > > Oh, I didn't know this, thanks for heads up! > I agree that GFP_NOWAIT is the best choice then. > > Btw, we probably shouldn't even add GFP_NOWAIT if the allocation is performed > from the bpf syscall context. Why would we fail to pre-allocate a map if > we can easily go into the reclaim? But probably better to leave it for > a separate change. The places affected by this patch are in atomic context. Prealloc path from syscall is using GFP_USER.
On Wed, Jul 06, 2022 at 07:09:22PM -0700, Alexei Starovoitov wrote: > On Wed, Jul 6, 2022 at 5:25 PM Roman Gushchin <roman.gushchin@linux.dev> wrote: > > > > On Thu, Jul 07, 2022 at 12:07:21AM +0000, Shakeel Butt wrote: > > > On Wed, Jul 06, 2022 at 03:58:47PM +0000, Yafang Shao wrote: > > > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > > > easily break the memcg limit by force charge. So it is very dangerous to > > > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > > > too much memory. > > > > > > Please use GFP_NOWAIT instead of (__GFP_ATOMIC | __GFP_KSWAPD_RECLAIM). > > > There is already a plan to completely remove __GFP_ATOMIC and mm-tree > > > already have a patch for that. > > > > Oh, I didn't know this, thanks for heads up! > > I agree that GFP_NOWAIT is the best choice then. > > > > Btw, we probably shouldn't even add GFP_NOWAIT if the allocation is performed > > from the bpf syscall context. Why would we fail to pre-allocate a map if > > we can easily go into the reclaim? But probably better to leave it for > > a separate change. > > The places affected by this patch are in atomic context. > Prealloc path from syscall is using GFP_USER. Right. Sorry, my bad, for some reason I was under an impression it's a common path for all allocations. Thanks!
On Thu, Jul 7, 2022 at 8:07 AM Shakeel Butt <shakeelb@google.com> wrote: > > On Wed, Jul 06, 2022 at 03:58:47PM +0000, Yafang Shao wrote: > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > easily break the memcg limit by force charge. So it is very dangerous to > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > too much memory. > > Please use GFP_NOWAIT instead of (__GFP_ATOMIC | __GFP_KSWAPD_RECLAIM). > There is already a plan to completely remove __GFP_ATOMIC and mm-tree > already have a patch for that. > After reading the discussion[1], it looks good to me to use GFP_NOWAIT instead. I will update it. [1]. https://lore.kernel.org/linux-mm/163712397076.13692.4727608274002939094@noble.neil.brown.name/ > > > > We introduced BPF_F_NO_PREALLOC is because full map pre-allocation is > > too memory expensive for some cases. That means removing __GFP_HIGH > > doesn't break the rule of BPF_F_NO_PREALLOC, but has the same goal with > > it-avoiding issues caused by too much memory. So let's remove it. > > > > The force charge of GFP_ATOMIC was introduced in > > commit 869712fd3de5 ("mm: memcontrol: fix network errors from failing > > __GFP_ATOMIC charges") by checking __GFP_ATOMIC, then got improved in > > commit 1461e8c2b6af ("memcg: unify force charging conditions") by > > checking __GFP_HIGH (that is no problem because both __GFP_HIGH and > > __GFP_ATOMIC are set in GFP_AOMIC). So, if we want to fix it in memcg, > > we have to carefully verify all the callsites. Now that we can fix it in > > BPF, we'd better not modify the memcg code. > > > > This fix can also apply to other run-time allocations, for example, the > > allocation in lpm trie, local storage and devmap. So let fix it > > consistently over the bpf code > > > > __GFP_KSWAPD_RECLAIM doesn't cooperate well with memcg pressure neither > > currently. But the memcg code can be improved to make > > __GFP_KSWAPD_RECLAIM work well under memcg pressure if desired. > > > > IMO there is no need to give all this detail and background on > GFP_ATOMIC and __GFP_KSWAPD_RECLAIM. Just say kernel allows GFP_ATOMIC > allocations to exceed memcg limits which we don't want in this case. So, > replace with GFP_NOWAIT which obey memcg limits. Both of these flags > tell kernel that the caller can not sleep. > Sure, thanks.
On Thu, Jul 7, 2022 at 3:28 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > On Thu, Jul 7, 2022 at 8:07 AM Shakeel Butt <shakeelb@google.com> wrote: > > > > On Wed, Jul 06, 2022 at 03:58:47PM +0000, Yafang Shao wrote: > > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > > easily break the memcg limit by force charge. So it is very dangerous to > > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > > too much memory. > > > > Please use GFP_NOWAIT instead of (__GFP_ATOMIC | __GFP_KSWAPD_RECLAIM). > > There is already a plan to completely remove __GFP_ATOMIC and mm-tree > > already have a patch for that. > > > > After reading the discussion[1], it looks good to me to use GFP_NOWAIT > instead. I will update it. Should we use GFP_ATOMIC | __GFP_NOMEMALLOC instead to align with its usage in the networking stack?
On Thu, Jul 7, 2022 at 11:44 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Jul 7, 2022 at 3:28 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > On Thu, Jul 7, 2022 at 8:07 AM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > On Wed, Jul 06, 2022 at 03:58:47PM +0000, Yafang Shao wrote: > > > > GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially > > > > if we allocate too much GFP_ATOMIC memory. For example, when we set the > > > > memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can > > > > easily break the memcg limit by force charge. So it is very dangerous to > > > > use GFP_ATOMIC in non-preallocated case. One way to make it safe is to > > > > remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | > > > > __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate > > > > too much memory. > > > > > > Please use GFP_NOWAIT instead of (__GFP_ATOMIC | __GFP_KSWAPD_RECLAIM). > > > There is already a plan to completely remove __GFP_ATOMIC and mm-tree > > > already have a patch for that. > > > > > > > After reading the discussion[1], it looks good to me to use GFP_NOWAIT > > instead. I will update it. > > Should we use GFP_ATOMIC | __GFP_NOMEMALLOC instead > to align with its usage in the networking stack? GFP_ATOMIC | __GFP_NOMEMALLOC will continue to break the memcg limit, so we have to modify the try_charge_memcg() code to make sure __GFP_NOMEMALLOC takes precedence over the __GFP_HIGH flag, IOW, if both of them are set we won't allow it to break memcg limit. That will need more verification.
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index c2867068e5bd..7672946126d5 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -845,7 +845,8 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net, struct bpf_dtab_netdev *dev; dev = bpf_map_kmalloc_node(&dtab->map, sizeof(*dev), - GFP_ATOMIC | __GFP_NOWARN, + __GFP_ATOMIC | __GFP_NOWARN | + __GFP_KSWAPD_RECLAIM, dtab->map.numa_node); if (!dev) return ERR_PTR(-ENOMEM); diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c index 17fb69c0e0dc..9d4559a1c032 100644 --- a/kernel/bpf/hashtab.c +++ b/kernel/bpf/hashtab.c @@ -61,7 +61,7 @@ * * As regular device interrupt handlers and soft interrupts are forced into * thread context, the existing code which does - * spin_lock*(); alloc(GPF_ATOMIC); spin_unlock*(); + * spin_lock*(); alloc(GFP_ATOMIC); spin_unlock*(); * just works. * * In theory the BPF locks could be converted to regular spinlocks as well, @@ -978,7 +978,8 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key, goto dec_count; } l_new = bpf_map_kmalloc_node(&htab->map, htab->elem_size, - GFP_ATOMIC | __GFP_NOWARN, + __GFP_ATOMIC | __GFP_NOWARN | + __GFP_KSWAPD_RECLAIM, htab->map.numa_node); if (!l_new) { l_new = ERR_PTR(-ENOMEM); @@ -996,7 +997,8 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key, } else { /* alloc_percpu zero-fills */ pptr = bpf_map_alloc_percpu(&htab->map, size, 8, - GFP_ATOMIC | __GFP_NOWARN); + __GFP_ATOMIC | __GFP_NOWARN | + __GFP_KSWAPD_RECLAIM); if (!pptr) { kfree(l_new); l_new = ERR_PTR(-ENOMEM); diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c index 8654fc97f5fe..534b69682b17 100644 --- a/kernel/bpf/local_storage.c +++ b/kernel/bpf/local_storage.c @@ -165,7 +165,8 @@ static int cgroup_storage_update_elem(struct bpf_map *map, void *key, } new = bpf_map_kmalloc_node(map, struct_size(new, data, map->value_size), - __GFP_ZERO | GFP_ATOMIC | __GFP_NOWARN, + __GFP_ZERO | __GFP_ATOMIC | __GFP_NOWARN | + __GFP_KSWAPD_RECLAIM, map->numa_node); if (!new) return -ENOMEM; diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c index f0d05a3cc4b9..7bae7133f1dd 100644 --- a/kernel/bpf/lpm_trie.c +++ b/kernel/bpf/lpm_trie.c @@ -285,7 +285,8 @@ static struct lpm_trie_node *lpm_trie_node_alloc(const struct lpm_trie *trie, if (value) size += trie->map.value_size; - node = bpf_map_kmalloc_node(&trie->map, size, GFP_ATOMIC | __GFP_NOWARN, + node = bpf_map_kmalloc_node(&trie->map, size, __GFP_ATOMIC | + __GFP_KSWAPD_RECLAIM | __GFP_NOWARN, trie->map.numa_node); if (!node) return NULL;
GFP_ATOMIC doesn't cooperate well with memcg pressure so far, especially if we allocate too much GFP_ATOMIC memory. For example, when we set the memcg limit to limit a non-preallocated bpf memory, the GFP_ATOMIC can easily break the memcg limit by force charge. So it is very dangerous to use GFP_ATOMIC in non-preallocated case. One way to make it safe is to remove __GFP_HIGH from GFP_ATOMIC, IOW, use (__GFP_ATOMIC | __GFP_KSWAPD_RECLAIM) instead, then it will be limited if we allocate too much memory. We introduced BPF_F_NO_PREALLOC is because full map pre-allocation is too memory expensive for some cases. That means removing __GFP_HIGH doesn't break the rule of BPF_F_NO_PREALLOC, but has the same goal with it-avoiding issues caused by too much memory. So let's remove it. The force charge of GFP_ATOMIC was introduced in commit 869712fd3de5 ("mm: memcontrol: fix network errors from failing __GFP_ATOMIC charges") by checking __GFP_ATOMIC, then got improved in commit 1461e8c2b6af ("memcg: unify force charging conditions") by checking __GFP_HIGH (that is no problem because both __GFP_HIGH and __GFP_ATOMIC are set in GFP_AOMIC). So, if we want to fix it in memcg, we have to carefully verify all the callsites. Now that we can fix it in BPF, we'd better not modify the memcg code. This fix can also apply to other run-time allocations, for example, the allocation in lpm trie, local storage and devmap. So let fix it consistently over the bpf code __GFP_KSWAPD_RECLAIM doesn't cooperate well with memcg pressure neither currently. But the memcg code can be improved to make __GFP_KSWAPD_RECLAIM work well under memcg pressure if desired. It also fixes a typo in the comment. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev> --- kernel/bpf/devmap.c | 3 ++- kernel/bpf/hashtab.c | 8 +++++--- kernel/bpf/local_storage.c | 3 ++- kernel/bpf/lpm_trie.c | 3 ++- 4 files changed, 11 insertions(+), 6 deletions(-)