| Message ID | 20170126115833.GI6590@dhcp22.suse.cz |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On 01/26/2017 12:58 PM, Michal Hocko wrote: > On Thu 26-01-17 12:33:55, Daniel Borkmann wrote: >> On 01/26/2017 11:08 AM, Michal Hocko wrote: > [...] >>> If you disagree I can drop the bpf part of course... >> >> If we could consolidate these spots with kvmalloc() eventually, I'm >> all for it. But even if __GFP_NORETRY is not covered down to all >> possible paths, it kind of does have an effect already of saying >> 'don't try too hard', so would it be harmful to still keep that for >> now? If it's not, I'd personally prefer to just leave it as is until >> there's some form of support by kvmalloc() and friends. > > Well, you can use kvmalloc(size, GFP_KERNEL|__GFP_NORETRY). It is not > disallowed. It is not _supported_ which means that if it doesn't work as > you expect you are on your own. Which is actually the situation right > now as well. But I still think that this is just not right thing to do. > Even though it might happen to work in some cases it gives a false > impression of a solution. So I would rather go with Hmm. 'On my own' means, we could potentially BUG somewhere down the vmalloc implementation, etc, presumably? So it might in-fact be harmful to pass that, right? > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 8697f43cf93c..a6dc4d596f14 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -53,6 +53,11 @@ void bpf_register_map_type(struct bpf_map_type_list *tl) > > void *bpf_map_area_alloc(size_t size) > { > + /* > + * FIXME: we would really like to not trigger the OOM killer and rather > + * fail instead. This is not supported right now. Please nag MM people > + * if these OOM start bothering people. > + */ Ok, I know this is out of scope for this series, but since i) this is _not_ the _only_ spot right now which has such a construct and ii) I am already kind of nagging a bit ;), my question would be, what would it take to start supporting it? > return kvzalloc(size, GFP_USER); > } Thanks, Daniel
On Thu 26-01-17 14:10:06, Daniel Borkmann wrote: > On 01/26/2017 12:58 PM, Michal Hocko wrote: > > On Thu 26-01-17 12:33:55, Daniel Borkmann wrote: > > > On 01/26/2017 11:08 AM, Michal Hocko wrote: > > [...] > > > > If you disagree I can drop the bpf part of course... > > > > > > If we could consolidate these spots with kvmalloc() eventually, I'm > > > all for it. But even if __GFP_NORETRY is not covered down to all > > > possible paths, it kind of does have an effect already of saying > > > 'don't try too hard', so would it be harmful to still keep that for > > > now? If it's not, I'd personally prefer to just leave it as is until > > > there's some form of support by kvmalloc() and friends. > > > > Well, you can use kvmalloc(size, GFP_KERNEL|__GFP_NORETRY). It is not > > disallowed. It is not _supported_ which means that if it doesn't work as > > you expect you are on your own. Which is actually the situation right > > now as well. But I still think that this is just not right thing to do. > > Even though it might happen to work in some cases it gives a false > > impression of a solution. So I would rather go with > > Hmm. 'On my own' means, we could potentially BUG somewhere down the > vmalloc implementation, etc, presumably? So it might in-fact be > harmful to pass that, right? No it would mean that it might eventually hit the behavior which you are trying to avoid - in other words it may invoke OOM killer even though __GFP_NORETRY means giving up before any system wide disruptive actions a re taken. > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 8697f43cf93c..a6dc4d596f14 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -53,6 +53,11 @@ void bpf_register_map_type(struct bpf_map_type_list *tl) > > > > void *bpf_map_area_alloc(size_t size) > > { > > + /* > > + * FIXME: we would really like to not trigger the OOM killer and rather > > + * fail instead. This is not supported right now. Please nag MM people > > + * if these OOM start bothering people. > > + */ > > Ok, I know this is out of scope for this series, but since i) this > is _not_ the _only_ spot right now which has such a construct and ii) > I am already kind of nagging a bit ;), my question would be, what > would it take to start supporting it? propagate gfp mask all the way down from vmalloc to all places which might allocate down the path and especially page table allocation function are PITA because they are really deep. This is a lot of work... But realistically, how big is this problem really? Is it really worth it? You said this is an admin only interface and admin can kill the machine by OOM and other means already. Moreover and I should probably mention it explicitly, your d407bd25a204b reduced the likelyhood of oom for other reason. kmalloc used GPF_USER previously and with order > 0 && order <= PAGE_ALLOC_COSTLY_ORDER this could indeed hit the OOM e.g. due to memory fragmentation. It would be much harder to hit the OOM killer from vmalloc which doesn't issue higher order allocation requests. Or have you ever seen the OOM killer pointing to the vmalloc fallback path?
On 01/26/2017 02:40 PM, Michal Hocko wrote: > On Thu 26-01-17 14:10:06, Daniel Borkmann wrote: >> On 01/26/2017 12:58 PM, Michal Hocko wrote: >>> On Thu 26-01-17 12:33:55, Daniel Borkmann wrote: >>>> On 01/26/2017 11:08 AM, Michal Hocko wrote: >>> [...] >>>>> If you disagree I can drop the bpf part of course... >>>> >>>> If we could consolidate these spots with kvmalloc() eventually, I'm >>>> all for it. But even if __GFP_NORETRY is not covered down to all >>>> possible paths, it kind of does have an effect already of saying >>>> 'don't try too hard', so would it be harmful to still keep that for >>>> now? If it's not, I'd personally prefer to just leave it as is until >>>> there's some form of support by kvmalloc() and friends. >>> >>> Well, you can use kvmalloc(size, GFP_KERNEL|__GFP_NORETRY). It is not >>> disallowed. It is not _supported_ which means that if it doesn't work as >>> you expect you are on your own. Which is actually the situation right >>> now as well. But I still think that this is just not right thing to do. >>> Even though it might happen to work in some cases it gives a false >>> impression of a solution. So I would rather go with >> >> Hmm. 'On my own' means, we could potentially BUG somewhere down the >> vmalloc implementation, etc, presumably? So it might in-fact be >> harmful to pass that, right? > > No it would mean that it might eventually hit the behavior which you are > trying to avoid - in other words it may invoke OOM killer even though > __GFP_NORETRY means giving up before any system wide disruptive actions > a re taken. Ok, thanks for clarifying, more on that further below. >>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c >>> index 8697f43cf93c..a6dc4d596f14 100644 >>> --- a/kernel/bpf/syscall.c >>> +++ b/kernel/bpf/syscall.c >>> @@ -53,6 +53,11 @@ void bpf_register_map_type(struct bpf_map_type_list *tl) >>> >>> void *bpf_map_area_alloc(size_t size) >>> { >>> + /* >>> + * FIXME: we would really like to not trigger the OOM killer and rather >>> + * fail instead. This is not supported right now. Please nag MM people >>> + * if these OOM start bothering people. >>> + */ >> >> Ok, I know this is out of scope for this series, but since i) this >> is _not_ the _only_ spot right now which has such a construct and ii) >> I am already kind of nagging a bit ;), my question would be, what >> would it take to start supporting it? > > propagate gfp mask all the way down from vmalloc to all places which > might allocate down the path and especially page table allocation > function are PITA because they are really deep. This is a lot of work... > > But realistically, how big is this problem really? Is it really worth > it? You said this is an admin only interface and admin can kill the > machine by OOM and other means already. > > Moreover and I should probably mention it explicitly, your d407bd25a204b > reduced the likelyhood of oom for other reason. kmalloc used GPF_USER > previously and with order > 0 && order <= PAGE_ALLOC_COSTLY_ORDER this > could indeed hit the OOM e.g. due to memory fragmentation. It would be > much harder to hit the OOM killer from vmalloc which doesn't issue > higher order allocation requests. Or have you ever seen the OOM killer > pointing to the vmalloc fallback path? The case I was concerned about was from vmalloc() path, not kmalloc(). That was where the stack trace indicating OOM pointed to. As an example, there could be really large allocation requests for maps where the map has pre-allocated memory for its elements. Thus, if we get to the point where we need to kill others due to shortage of mem for satisfying this, I'd much much rather prefer to just not let vmalloc() work really hard and fail early on instead. In my (crafted) test case, I was connected via ssh and it each time reliably killed my connection, which is really suboptimal. F.e., I could also imagine a buggy or miscalculated map definition for a prog that is provisioned to multiple places, which then accidentally triggers this. Or if large on purpose, but we crossed the line, it could be handled more gracefully, f.e. I could imagine an option to falling back to a non-pre-allocated map flavor from the application loading the program. Trade-off for sure, but still allowing it to operate up to a certain extend. Granted, if vmalloc() succeeded without trying hard and we then OOM elsewhere, too bad, but we don't have much control over that one anyway, only about our own request. Reason I asked above was whether having __GFP_NORETRY in would be fatal somewhere down the path, but seems not as you say. So to answer your second email with the bpf and netfilter hunks, why not replacing them with kvmalloc() and __GFP_NORETRY flag and add that big fat FIXME comment above there, saying explicitly that __GFP_NORETRY is not harmful though has only /partial/ effect right now and that full support needs to be implemented in future. That would still be better that not having it, imo, and the FIXME would make expectations clear to anyone reading that code. Thanks, Daniel
On Thu 26-01-17 21:34:04, Daniel Borkmann wrote: > On 01/26/2017 02:40 PM, Michal Hocko wrote: [...] > > But realistically, how big is this problem really? Is it really worth > > it? You said this is an admin only interface and admin can kill the > > machine by OOM and other means already. > > > > Moreover and I should probably mention it explicitly, your d407bd25a204b > > reduced the likelyhood of oom for other reason. kmalloc used GPF_USER > > previously and with order > 0 && order <= PAGE_ALLOC_COSTLY_ORDER this > > could indeed hit the OOM e.g. due to memory fragmentation. It would be > > much harder to hit the OOM killer from vmalloc which doesn't issue > > higher order allocation requests. Or have you ever seen the OOM killer > > pointing to the vmalloc fallback path? > > The case I was concerned about was from vmalloc() path, not kmalloc(). > That was where the stack trace indicating OOM pointed to. As an example, > there could be really large allocation requests for maps where the map > has pre-allocated memory for its elements. Thus, if we get to the point > where we need to kill others due to shortage of mem for satisfying this, > I'd much much rather prefer to just not let vmalloc() work really hard > and fail early on instead. I see, but as already mentioned, chances are that by the time you get close to the OOM somebody else will hit the OOM before the vmalloc path manages to free the allocated memory. > In my (crafted) test case, I was connected > via ssh and it each time reliably killed my connection, which is really > suboptimal. > > F.e., I could also imagine a buggy or miscalculated map definition for > a prog that is provisioned to multiple places, which then accidentally > triggers this. Or if large on purpose, but we crossed the line, it > could be handled more gracefully, f.e. I could imagine an option to > falling back to a non-pre-allocated map flavor from the application > loading the program. Trade-off for sure, but still allowing it to > operate up to a certain extend. Granted, if vmalloc() succeeded without > trying hard and we then OOM elsewhere, too bad, but we don't have much > control over that one anyway, only about our own request. Reason I > asked above was whether having __GFP_NORETRY in would be fatal > somewhere down the path, but seems not as you say. > > So to answer your second email with the bpf and netfilter hunks, why > not replacing them with kvmalloc() and __GFP_NORETRY flag and add that > big fat FIXME comment above there, saying explicitly that __GFP_NORETRY > is not harmful though has only /partial/ effect right now and that full > support needs to be implemented in future. That would still be better > that not having it, imo, and the FIXME would make expectations clear > to anyone reading that code. Well, we can do that, I just would like to prevent from this (ab)use if there is no _real_ and _sensible_ usecase for it. Having a real bug report or a fallback mechanism you are mentioning above would justify the (ab)use IMHO. But that abuse would be documented properly and have a real reason to exist. That sounds like a better approach to me. But if you absolutely _insist_ I can change that.
On 01/27/2017 11:05 AM, Michal Hocko wrote: > On Thu 26-01-17 21:34:04, Daniel Borkmann wrote: >> On 01/26/2017 02:40 PM, Michal Hocko wrote: > [...] >>> But realistically, how big is this problem really? Is it really worth >>> it? You said this is an admin only interface and admin can kill the >>> machine by OOM and other means already. >>> >>> Moreover and I should probably mention it explicitly, your d407bd25a204b >>> reduced the likelyhood of oom for other reason. kmalloc used GPF_USER >>> previously and with order > 0 && order <= PAGE_ALLOC_COSTLY_ORDER this >>> could indeed hit the OOM e.g. due to memory fragmentation. It would be >>> much harder to hit the OOM killer from vmalloc which doesn't issue >>> higher order allocation requests. Or have you ever seen the OOM killer >>> pointing to the vmalloc fallback path? >> >> The case I was concerned about was from vmalloc() path, not kmalloc(). >> That was where the stack trace indicating OOM pointed to. As an example, >> there could be really large allocation requests for maps where the map >> has pre-allocated memory for its elements. Thus, if we get to the point >> where we need to kill others due to shortage of mem for satisfying this, >> I'd much much rather prefer to just not let vmalloc() work really hard >> and fail early on instead. > > I see, but as already mentioned, chances are that by the time you get > close to the OOM somebody else will hit the OOM before the vmalloc path > manages to free the allocated memory. > >> In my (crafted) test case, I was connected >> via ssh and it each time reliably killed my connection, which is really >> suboptimal. >> >> F.e., I could also imagine a buggy or miscalculated map definition for >> a prog that is provisioned to multiple places, which then accidentally >> triggers this. Or if large on purpose, but we crossed the line, it >> could be handled more gracefully, f.e. I could imagine an option to >> falling back to a non-pre-allocated map flavor from the application >> loading the program. Trade-off for sure, but still allowing it to >> operate up to a certain extend. Granted, if vmalloc() succeeded without >> trying hard and we then OOM elsewhere, too bad, but we don't have much >> control over that one anyway, only about our own request. Reason I >> asked above was whether having __GFP_NORETRY in would be fatal >> somewhere down the path, but seems not as you say. >> >> So to answer your second email with the bpf and netfilter hunks, why >> not replacing them with kvmalloc() and __GFP_NORETRY flag and add that >> big fat FIXME comment above there, saying explicitly that __GFP_NORETRY >> is not harmful though has only /partial/ effect right now and that full >> support needs to be implemented in future. That would still be better >> that not having it, imo, and the FIXME would make expectations clear >> to anyone reading that code. > > Well, we can do that, I just would like to prevent from this (ab)use > if there is no _real_ and _sensible_ usecase for it. Having a real bug Understandable. > report or a fallback mechanism you are mentioning above would justify > the (ab)use IMHO. But that abuse would be documented properly and have a > real reason to exist. That sounds like a better approach to me. > > But if you absolutely _insist_ I can change that. Yeah, please do (with a big FIXME comment as mentioned), this originally came from a real bug report. Anyway, feel free to add my Acked-by then. Thanks again, Daniel
On Fri 27-01-17 21:12:26, Daniel Borkmann wrote: > On 01/27/2017 11:05 AM, Michal Hocko wrote: > > On Thu 26-01-17 21:34:04, Daniel Borkmann wrote: [...] > > > So to answer your second email with the bpf and netfilter hunks, why > > > not replacing them with kvmalloc() and __GFP_NORETRY flag and add that > > > big fat FIXME comment above there, saying explicitly that __GFP_NORETRY > > > is not harmful though has only /partial/ effect right now and that full > > > support needs to be implemented in future. That would still be better > > > that not having it, imo, and the FIXME would make expectations clear > > > to anyone reading that code. > > > > Well, we can do that, I just would like to prevent from this (ab)use > > if there is no _real_ and _sensible_ usecase for it. Having a real bug > > Understandable. > > > report or a fallback mechanism you are mentioning above would justify > > the (ab)use IMHO. But that abuse would be documented properly and have a > > real reason to exist. That sounds like a better approach to me. > > > > But if you absolutely _insist_ I can change that. > > Yeah, please do (with a big FIXME comment as mentioned), this originally > came from a real bug report. Anyway, feel free to add my Acked-by then. Thanks! I will repost the whole series today.
On 01/30/2017 08:56 AM, Michal Hocko wrote: > On Fri 27-01-17 21:12:26, Daniel Borkmann wrote: >> On 01/27/2017 11:05 AM, Michal Hocko wrote: >>> On Thu 26-01-17 21:34:04, Daniel Borkmann wrote: > [...] >>>> So to answer your second email with the bpf and netfilter hunks, why >>>> not replacing them with kvmalloc() and __GFP_NORETRY flag and add that >>>> big fat FIXME comment above there, saying explicitly that __GFP_NORETRY >>>> is not harmful though has only /partial/ effect right now and that full >>>> support needs to be implemented in future. That would still be better >>>> that not having it, imo, and the FIXME would make expectations clear >>>> to anyone reading that code. >>> >>> Well, we can do that, I just would like to prevent from this (ab)use >>> if there is no _real_ and _sensible_ usecase for it. Having a real bug >> >> Understandable. >> >>> report or a fallback mechanism you are mentioning above would justify >>> the (ab)use IMHO. But that abuse would be documented properly and have a >>> real reason to exist. That sounds like a better approach to me. >>> >>> But if you absolutely _insist_ I can change that. >> >> Yeah, please do (with a big FIXME comment as mentioned), this originally >> came from a real bug report. Anyway, feel free to add my Acked-by then. > > Thanks! I will repost the whole series today. Looks like I got only Cc'ed on the cover letter of your v3 from today (should have been v4 actually?). Anyway, I looked up the last patch on lkml [1] and it seems you forgot the __GFP_NORETRY we talked about? At least that was what was discussed above (insisting on __GFP_NORETRY plus FIXME comment) for providing my Acked-by then. Can you still fix that up in a final respin? Thanks again, Daniel [1] https://lkml.org/lkml/2017/1/30/129
On Mon 30-01-17 17:15:08, Daniel Borkmann wrote: > On 01/30/2017 08:56 AM, Michal Hocko wrote: > > On Fri 27-01-17 21:12:26, Daniel Borkmann wrote: > > > On 01/27/2017 11:05 AM, Michal Hocko wrote: > > > > On Thu 26-01-17 21:34:04, Daniel Borkmann wrote: > > [...] > > > > > So to answer your second email with the bpf and netfilter hunks, why > > > > > not replacing them with kvmalloc() and __GFP_NORETRY flag and add that > > > > > big fat FIXME comment above there, saying explicitly that __GFP_NORETRY > > > > > is not harmful though has only /partial/ effect right now and that full > > > > > support needs to be implemented in future. That would still be better > > > > > that not having it, imo, and the FIXME would make expectations clear > > > > > to anyone reading that code. > > > > > > > > Well, we can do that, I just would like to prevent from this (ab)use > > > > if there is no _real_ and _sensible_ usecase for it. Having a real bug > > > > > > Understandable. > > > > > > > report or a fallback mechanism you are mentioning above would justify > > > > the (ab)use IMHO. But that abuse would be documented properly and have a > > > > real reason to exist. That sounds like a better approach to me. > > > > > > > > But if you absolutely _insist_ I can change that. > > > > > > Yeah, please do (with a big FIXME comment as mentioned), this originally > > > came from a real bug report. Anyway, feel free to add my Acked-by then. > > > > Thanks! I will repost the whole series today. > > Looks like I got only Cc'ed on the cover letter of your v3 from today > (should have been v4 actually?). Yes > Anyway, I looked up the last patch > on lkml [1] and it seems you forgot the __GFP_NORETRY we talked about? I misread your response. I thought you were OK with the FIXME explanation. > At least that was what was discussed above (insisting on __GFP_NORETRY > plus FIXME comment) for providing my Acked-by then. Can you still fix > that up in a final respin? I will probably just drop that last patch instead. I am not convinced that we should bend the new API over and let people mimic that throughout the code. I have just seen too many examples of this pattern already. I would also like to prevent the next rebase, unless there any issues with some patches of course.
On 01/30/2017 05:28 PM, Michal Hocko wrote: > On Mon 30-01-17 17:15:08, Daniel Borkmann wrote: >> On 01/30/2017 08:56 AM, Michal Hocko wrote: >>> On Fri 27-01-17 21:12:26, Daniel Borkmann wrote: >>>> On 01/27/2017 11:05 AM, Michal Hocko wrote: >>>>> On Thu 26-01-17 21:34:04, Daniel Borkmann wrote: >>> [...] >>>>>> So to answer your second email with the bpf and netfilter hunks, why >>>>>> not replacing them with kvmalloc() and __GFP_NORETRY flag and add that >>>>>> big fat FIXME comment above there, saying explicitly that __GFP_NORETRY >>>>>> is not harmful though has only /partial/ effect right now and that full >>>>>> support needs to be implemented in future. That would still be better >>>>>> that not having it, imo, and the FIXME would make expectations clear >>>>>> to anyone reading that code. >>>>> >>>>> Well, we can do that, I just would like to prevent from this (ab)use >>>>> if there is no _real_ and _sensible_ usecase for it. Having a real bug >>>> >>>> Understandable. >>>> >>>>> report or a fallback mechanism you are mentioning above would justify >>>>> the (ab)use IMHO. But that abuse would be documented properly and have a >>>>> real reason to exist. That sounds like a better approach to me. >>>>> >>>>> But if you absolutely _insist_ I can change that. >>>> >>>> Yeah, please do (with a big FIXME comment as mentioned), this originally >>>> came from a real bug report. Anyway, feel free to add my Acked-by then. >>> >>> Thanks! I will repost the whole series today. >> >> Looks like I got only Cc'ed on the cover letter of your v3 from today >> (should have been v4 actually?). > > Yes > >> Anyway, I looked up the last patch >> on lkml [1] and it seems you forgot the __GFP_NORETRY we talked about? > > I misread your response. I thought you were OK with the FIXME > explanation. > >> At least that was what was discussed above (insisting on __GFP_NORETRY >> plus FIXME comment) for providing my Acked-by then. Can you still fix >> that up in a final respin? > > I will probably just drop that last patch instead. I am not convinced > that we should bend the new API over and let people mimic that > throughout the code. I have just seen too many examples of this pattern > already. > > I would also like to prevent the next rebase, unless there any issues > with some patches of course. Ok, I'm fine with that as well. Thanks, Daniel
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 8697f43cf93c..a6dc4d596f14 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -53,6 +53,11 @@ void bpf_register_map_type(struct bpf_map_type_list *tl) void *bpf_map_area_alloc(size_t size) { + /* + * FIXME: we would really like to not trigger the OOM killer and rather + * fail instead. This is not supported right now. Please nag MM people + * if these OOM start bothering people. + */ return kvzalloc(size, GFP_USER); }