Hi,
This series is the result of the discussion with Linus around ipc
subsystem initialization and how it behaves with error return when
calling rhashtable_init()[1]. Instead of caring about the error
or calling the infamous BUG_ON, Linus suggested we guarantee the
rhashtable allocation.
First two patches modify rhashtable_init() to just return 0, future
patches I guess can update more callers.
patch 3 is a nit I found while reading the code and just makes use
kvmalloc_array().
patch 4+5 remove some ipc hacks we no longer need.
patch 6 updates the rhashtable test module. Trivial.
Please consider for v4.18.
Thanks!
[0] https://lkml.org/lkml/2018/5/23/758
Davidlohr Bueso (6):
lib/rhashtable: convert param sanitations to WARN_ON
lib/rhashtable: guarantee initial hashtable allocation
lib/bucket_locks: use kvmalloc_array()
ipc: get rid of ids->tables_initialized hack
ipc: simplify ipc initialization
lib/test_rhashtable: rhashtable_init() can no longer fail
include/linux/ipc_namespace.h | 1 -
ipc/msg.c | 9 ++++----
ipc/namespace.c | 20 ++++--------------
ipc/sem.c | 10 ++++-----
ipc/shm.c | 9 ++++----
ipc/util.c | 41 ++++++++++++------------------------
ipc/util.h | 18 ++++++++--------
lib/bucket_locks.c | 2 +-
lib/rhashtable.c | 49 ++++++++++++++++++++++++++++++++++---------
lib/test_rhashtable.c | 6 +-----
10 files changed, 79 insertions(+), 86 deletions(-)
--
2.13.6
rhashtable_init() may fail due to -ENOMEM, thus making the
entire api unusable. This patch removes this scenario,
however unlikely. In order to guarantee memory allocation,
this patch refactors bucket_table_alloc() to add a 'retry'
parameter which always ends up doing GFP_KERNEL|__GFP_NOFAIL
for both the tbl as well as alloc_bucket_spinlocks().
So upon the first table allocation failure, we shrink the
size to the smallest value that makes sense and retry the alloc
with the same semantics. If we fail again, then we force the
call with __GFP_NOFAIL. With the defaults, this means that from
64 buckets, we retry with only 4. Any later issues regarding
performance due to collisions or larger table resizing (when
more memory becomes available) is the last of our problems.
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Davidlohr Bueso <redacted>
---
lib/rhashtable.c | 40 +++++++++++++++++++++++++++++++++++-----
1 file changed, 35 insertions(+), 5 deletions(-)
I'd prefer this logic to be moved to the caller. So just call the
function with GFP_KERNEL | __GFP_NOFAIL.
Of course you need to modify bucket_table_alloc so that it still
treats this as GFP_KERNEL (as opposed to GFP_ATOMIC). That is,
instead of
if (gfp != GFP_KERNEL)
You will need
if ((gfp & ~__GFP_NOFAIL) != GFP_KERNEL)
quoted hunk
@@ -1067,9 +1086,20 @@ int rhashtable_init(struct rhashtable *ht, } }+ /*+ * This is api initialization and thus we need to guarantee the+ * initial rhashtable allocation. Upon failure, retry with a+ * smallest possible size, otherwise we exhaust our options with+ * __GFP_NOFAIL.+ */ tbl = bucket_table_alloc(ht, size, GFP_KERNEL);- if (tbl == NULL)- return -ENOMEM;+ if (unlikely(tbl == NULL)) {+ size = HASH_MIN_SIZE;++ tbl = bucket_table_alloc(ht, size, GFP_KERNEL);+ if (tbl == NULL)+ tbl = bucket_table_alloc_retry(ht, size, GFP_KERNEL);+ }
+ /*
+ * This is api initialization and thus we need to guarantee the
+ * initial rhashtable allocation. Upon failure, retry with a
+ * smallest possible size, otherwise we exhaust our options with
+ * __GFP_NOFAIL.
+ */
tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
- if (tbl == NULL)
- return -ENOMEM;
+ if (unlikely(tbl == NULL)) {
+ size = HASH_MIN_SIZE;
+
+ tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
+ if (tbl == NULL)
+ tbl = bucket_table_alloc_retry(ht, size, GFP_KERNEL);
+ }
Perhaps you should also explain here why we don't just try the
minimum size with __GFP_NOFAIL as the second step rather than the
third.
Please see the comment above, I try to explain the rationale.
Thanks,
Davidlohr
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2018-05-29 18:04:42
On Tue, May 29, 2018 at 10:03:38AM -0700, Davidlohr Bueso wrote:
On Mon, 28 May 2018, Herbert Xu wrote:
quoted
quoted
+ /*
+ * This is api initialization and thus we need to guarantee the
+ * initial rhashtable allocation. Upon failure, retry with a
+ * smallest possible size, otherwise we exhaust our options with
+ * __GFP_NOFAIL.
+ */
tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
- if (tbl == NULL)
- return -ENOMEM;
+ if (unlikely(tbl == NULL)) {
+ size = HASH_MIN_SIZE;
+
+ tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
+ if (tbl == NULL)
+ tbl = bucket_table_alloc_retry(ht, size, GFP_KERNEL);
+ }
Perhaps you should also explain here why we don't just try the
minimum size with __GFP_NOFAIL as the second step rather than the
third.
Please see the comment above, I try to explain the rationale.
It doesn't explain it at all. In fact I don't see why we neeed
three attempts, just do the GFP_NOFAIL as the second and final step.
Second attempt is reduced size only as we don't want to GFP_NOFAIL
if we can avoid it helping the allocator. We go from an arbitrary
allocation to the smallest possible allocation, if all that fails
ok lets use GFP_NOFAIL. I don't know how this is not clear...
Thanks,
Davidlohr
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2018-05-29 18:28:03
On Tue, May 29, 2018 at 10:59:27AM -0700, Davidlohr Bueso wrote:
On Wed, 30 May 2018, Herbert Xu wrote:
quoted
It doesn't explain it at all. In fact I don't see why we neeed
three attempts, just do the GFP_NOFAIL as the second and final step.
Second attempt is reduced size only as we don't want to GFP_NOFAIL
if we can avoid it helping the allocator. We go from an arbitrary
allocation to the smallest possible allocation, if all that fails
ok lets use GFP_NOFAIL. I don't know how this is not clear...
That's exactly what you need to explain in the patch or the commit
message. In fact you still haven't explained it fully. Why do we
need a second attempt without the GFP_NOFAIL? How does it help the
allocator?
Cheers,
--
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, May 29, 2018 at 10:59:27AM -0700, Davidlohr Bueso wrote:
That's exactly what you need to explain in the patch or the commit
message. In fact you still haven't explained it fully. Why do we
need a second attempt without the GFP_NOFAIL? How does it help the
allocator?
It helps in that we have two fastpath attempts before going in to
__alloc_pages_slowpath() and looping in __GFP_NOFAIL. But yeah, I
see your point. We can just apply KISS and avoid the extra alloc.
That actually makes more sense to me now than ignoring min_size
based on simplicity.
Thanks for the review.
Thanks,
Davidlohr
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2018-05-28 10:02:50
On Thu, May 24, 2018 at 02:11:31PM -0700, Davidlohr Bueso wrote:
+ /*
+ * This is api initialization and thus we need to guarantee the
+ * initial rhashtable allocation. Upon failure, retry with a
+ * smallest possible size, otherwise we exhaust our options with
+ * __GFP_NOFAIL.
+ */
tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
- if (tbl == NULL)
- return -ENOMEM;
+ if (unlikely(tbl == NULL)) {
+ size = HASH_MIN_SIZE;
On Thu, May 24, 2018 at 02:11:31PM -0700, Davidlohr Bueso wrote:
quoted
+ /*
+ * This is api initialization and thus we need to guarantee the
+ * initial rhashtable allocation. Upon failure, retry with a
+ * smallest possible size, otherwise we exhaust our options with
+ * __GFP_NOFAIL.
+ */
tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
- if (tbl == NULL)
- return -ENOMEM;
+ if (unlikely(tbl == NULL)) {
+ size = HASH_MIN_SIZE;
You should also take min_size into account. Yes I know the current
code ignores it unless you also set nelem_hint. But that's just a
bug.
For the sake of simplicity, Linus suggested directly using HASH_MIN_SIZE
such that we have a single fallback.
Thanks,
Davidlohr
For the purpose of making rhashtable_init() unable to fail,
we can replace the returning -EINVAL with WARN_ONs whenever
the caller passes bogus parameters during initialization.
Signed-off-by: Davidlohr Bueso <redacted>
---
lib/rhashtable.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2018-05-28 09:41:04
On Thu, May 24, 2018 at 02:11:30PM -0700, Davidlohr Bueso wrote:
For the purpose of making rhashtable_init() unable to fail,
we can replace the returning -EINVAL with WARN_ONs whenever
the caller passes bogus parameters during initialization.
Signed-off-by: Davidlohr Bueso <redacted>
I don't really see why we need these WARN_ONs. The problem should
be quite obvious when you're writing your code.
If you really want them perhaps add them to the ipc code instead?
Well, I don't really _want_ them; nor does the ipc code which already
does a WARN_ON() (but that goes away in future patches). What I want
is to get rid of the return path. So I don't really care if we convert
them to WARN or remove them altogether. Would you be happy with the
later?
Thanks,
Davidlohr
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2018-05-28 15:55:55
On Mon, May 28, 2018 at 06:12:09AM -0700, Davidlohr Bueso wrote:
Well, I don't really _want_ them; nor does the ipc code which already
does a WARN_ON() (but that goes away in future patches). What I want
is to get rid of the return path. So I don't really care if we convert
them to WARN or remove them altogether. Would you be happy with the
later?
It has nothing to do with the error return path. Assuming you
remove the allocation failure path, then this error path can never
trigger for *your* use-case. IOW you can either just ignore the
return value for ipc or just do the WARN_ON in your code.
Cheers,
--
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, May 28, 2018 at 06:12:09AM -0700, Davidlohr Bueso wrote:
quoted
Well, I don't really _want_ them; nor does the ipc code which already
does a WARN_ON() (but that goes away in future patches). What I want
is to get rid of the return path. So I don't really care if we convert
them to WARN or remove them altogether. Would you be happy with the
later?
It has nothing to do with the error return path. Assuming you
remove the allocation failure path, then this error path can never
trigger for *your* use-case.
Why would this be triggered by any use case if the developer setup the
params correctly...? I don't see the point of not getting rid of the
EINVAL or wrapping around warning around it. Ultimately it would be
good to just have rhashtable_init() return void.
For some reason we don't use this call, but we rely just
fine on kmalloc_array(). Make both consistent.
Signed-off-by: Davidlohr Bueso <redacted>
---
lib/bucket_locks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -31,7 +31,7 @@ int alloc_bucket_spinlocks(spinlock_t **locks, unsigned int *locks_mask,if(sizeof(spinlock_t)!=0){if(gfpflags_allow_blocking(gfp))-tlocks=kvmalloc(size*sizeof(spinlock_t),gfp);+tlocks=kvmalloc_array(size,sizeof(spinlock_t),gfp);elsetlocks=kmalloc_array(size,sizeof(spinlock_t),gfp);if(!tlocks)
gfp);
Side note: how about we just move that "gfpflags_allow_blocking()" into
kvmalloc() instead, and make kvmalloc() generally usable?
Now we have that really odd situation where kvmalloc() takes gfp flags, but
to quote the comment:
* Any use of gfp flags outside of GFP_KERNEL should be consulted with mm
people.
and the code:
/*
* vmalloc uses GFP_KERNEL for some internal allocations (e.g page
tables)
* so the given set of flags has to be compatible.
*/
WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
which isn't really all that helpful. Do mm people really want to be
consulted about random uses?
Maybe we could just make the rule for kvmalloc() be to only fall back on
vmalloc for allocations that are
- larger than page size
- blocking and allow GFP_KERNEL (so basically that WARN_ON_ONCE() logic in
kvmalloc_node).
Hmm? Isn't that what everybody really *wants* kvmalloc() and friends to do?
Linus
gfp);
Side note: how about we just move that "gfpflags_allow_blocking()" into
kvmalloc() instead, and make kvmalloc() generally usable?
Now we have that really odd situation where kvmalloc() takes gfp flags, but
to quote the comment:
* Any use of gfp flags outside of GFP_KERNEL should be consulted with mm
people.
and the code:
/*
* vmalloc uses GFP_KERNEL for some internal allocations (e.g page
tables)
* so the given set of flags has to be compatible.
*/
WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
which isn't really all that helpful. Do mm people really want to be
consulted about random uses?
The purpose was to have a clean usage base after the conversion. If we
are growing a non-trivial use base which wants to use GFP_NOWAIT semantic
then sure we can make kvmalloc never fallback to vmallock. But see
below...
Maybe we could just make the rule for kvmalloc() be to only fall back on
vmalloc for allocations that are
- larger than page size
- blocking and allow GFP_KERNEL (so basically that WARN_ON_ONCE() logic in
kvmalloc_node).
Hmm? Isn't that what everybody really *wants* kvmalloc() and friends to do?
... Well, there are users who would like to use kvmalloc for
GFP_NOFS/GFP_NOIO context. Do we want them to fail more likely for
larger order rather than have them fixed (to either drop the NOFS
because it just has been blindly copied from a different code without
too much thinking or use the scope NOFS/NOIO API)? A warn_on tends to be
rather harsh but effective way to push maintainers fix their broken
code...
--
Michal Hocko
SUSE Labs
gfp);
Side note: how about we just move that "gfpflags_allow_blocking()" into
kvmalloc() instead, and make kvmalloc() generally usable?
Now we have that really odd situation where kvmalloc() takes gfp flags, but
to quote the comment:
* Any use of gfp flags outside of GFP_KERNEL should be consulted with mm
people.
and the code:
/*
* vmalloc uses GFP_KERNEL for some internal allocations (e.g page
tables)
* so the given set of flags has to be compatible.
*/
WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
which isn't really all that helpful. Do mm people really want to be
consulted about random uses?
The purpose was to have a clean usage base after the conversion. If we
are growing a non-trivial use base which wants to use GFP_NOWAIT semantic
then sure we can make kvmalloc never fallback to vmallock. But see
below...
quoted
Maybe we could just make the rule for kvmalloc() be to only fall back on
vmalloc for allocations that are
- larger than page size
- blocking and allow GFP_KERNEL (so basically that WARN_ON_ONCE() logic in
kvmalloc_node).
Hmm? Isn't that what everybody really *wants* kvmalloc() and friends to do?
... Well, there are users who would like to use kvmalloc for
GFP_NOFS/GFP_NOIO context. Do we want them to fail more likely for
larger order rather than have them fixed (to either drop the NOFS
because it just has been blindly copied from a different code without
too much thinking or use the scope NOFS/NOIO API)? A warn_on tends to be
rather harsh but effective way to push maintainers fix their broken
code...
On Tue, May 29, 2018 at 9:51 AM Michal Hocko [off-list ref] wrote:
In other words, what about the following?
+ WARN_ON_ONCE((flags & (__GFP_FS|__GFP_IO)) !=
(__GFP_FS|__GFP_IO));
I still don't understand the point of this warning.
It's only stupid. It basically says "this function is garbage, so let me
warn about the fact that I'm a moron".
If we all needed to warn about ourselves being morons, there would be a
hell of a lot of big blinking signs everywhere.
And the *ONLY* thing that warning has ever caused is just stupid code that
does
if (flags == GFP_KERNEL)
.. do kvmalloc ..
else
.. do kmalloc() ..
which is a *STUPID* pattern.
In other words, the WARN_ON() is bogus garbage. It's bogus exactly because
NOBODY CARES and all everybody will ever do is to just avoid it by writing
worse code.
The whole and ONLY point of "kvmalloc()" and friends is to make it easy to
write code and _not_ have those idiotic "let's do kmalloc or kvmalloc
depending on the phase of the moon" garbage. So the warning has literally
destroyed the only value that function has!
+ if (!gfpflags_allow_blocking(flags))
+ return NULL;
+
And no. Now all the code *above* this check is just wrong. All the code
that modifies the gfp_flags with the intention of falling back on vmalloc()
is just wrong, since we're not falling back on vmalloc any more.
So I really think the semantics should be simple:
- if we don't allow GFP_KERNEL, then the code becomes just "kmalloc()",
since there is no valid fallback to vmalloc. vmalloc does GFP_KERNEL.
- otherwise, we do what we used to do (except now the warning is gone,
because we already handled the case it warned about).
Nothing else. No stupid other cases. The *only* thing that function should
ask itself is "can I fall back on vmalloc or not", and if it can't then it
should just be a kmalloc.
Because otherwise we'll continue to have people that just check in the
caller instead.
Linus
From: Michal Hocko <mhocko@kernel.org> Date: 2018-05-30 07:42:23
On Tue 29-05-18 15:46:25, Linus Torvalds wrote:
[...]
The whole and ONLY point of "kvmalloc()" and friends is to make it easy to
write code and _not_ have those idiotic "let's do kmalloc or kvmalloc
depending on the phase of the moon" garbage. So the warning has literally
destroyed the only value that function has!
Well, I do agree but I've also seen terrible things while doing the
conversion when introducing kvmalloc.
So I admit that the defensive mode here is mostly inspired by existing
users of vmalloc(GFP_NOFS). They are simply wrong and not really
eager to be fixed from my experience. Now with kvmalloc fixing them
up silently it would get even less likely to get fixed because there
won't be any deadlock possible (compared to open coded kvmalloc like
ext4_kvmalloc for example).
My experience also tells me that most of those vmalloc NOFS users
simply do not need NOFS at all because there is no risk of the reclaim
recursion deadlocks. They are just used because of cargo cult which is
sad and it causes some subtle problems for the direct reclaim. I would
really like to eliminate those (e.g. see [1]). It is sad reality that
people tend to be more sensitive to WARN splats than "look this is wrong
albeit not critical in most cases).
[1] http://lkml.kernel.org/r/20180424162712.GL17484@dhcp22.suse.cz
That being sad, if you believe that silently fixing up a code like that
is a good idea we can do the following of course:
From c1a098e809a109800f9cfa63cb27fe9a78f3f316 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 30 May 2018 09:34:39 +0200
Subject: [PATCH] mm: kvmalloc does not fallback to vmalloc for incompatible
gfp flags
kvmalloc warned about incompatible gfp_mask to catch abusers (mostly
GFP_NOFS) with an intention that this will motivate authors of the code
to fix those. Linus argues that this just motivates people to do even
more hacks like
if (gfp == GFP_KERNEL)
kvmalloc
else
kmalloc
I haven't seen this happening but it is true that we can grow those in
future. Therefore Linus suggested to simply not fallback to vmalloc for
incompatible gfp flags and rather stick with the kmalloc path.
Requested-by: Linus Torvalds [off-list ref]
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/util.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
On Wed, May 30, 2018 at 2:42 AM Michal Hocko [off-list ref] wrote:
That being sad, if you believe that silently fixing up a code like that
is a good idea we can do the following of course:
Ack.
Except for:
Linus argues that this just motivates people to do even
more hacks like
if (gfp == GFP_KERNEL)
kvmalloc
else
kmalloc
I haven't seen this happening but it is true that we can grow those in
future.
This whole discussion came from the fact that YES, THIS IS ACTUALLY HAPPENING.
See lib/bucket_locks.c - it just uses gfpflags_allow_blocking()
instead of explicitly checking for GFP_KERNEL (probably because the
only two cases it actually deals with is GFP_ATOMIC and GFP_KERNEL).
Linus
From: Michal Hocko <mhocko@kernel.org> Date: 2018-05-31 15:29:59
On Thu 31-05-18 10:01:51, Linus Torvalds wrote:
On Wed, May 30, 2018 at 2:42 AM Michal Hocko [off-list ref] wrote:
quoted
That being sad, if you believe that silently fixing up a code like that
is a good idea we can do the following of course:
Ack.
Except for:
quoted
Linus argues that this just motivates people to do even
more hacks like
if (gfp == GFP_KERNEL)
kvmalloc
else
kmalloc
I haven't seen this happening but it is true that we can grow those in
future.
This whole discussion came from the fact that YES, THIS IS ACTUALLY HAPPENING.
See lib/bucket_locks.c - it just uses gfpflags_allow_blocking()
instead of explicitly checking for GFP_KERNEL (probably because the
only two cases it actually deals with is GFP_ATOMIC and GFP_KERNEL).
OK, I haven't noticed this one and will fix it up. So what about the
following?
From abc6ac9a690060d5ceda79e747c78d24cc7f2951 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 30 May 2018 09:34:39 +0200
Subject: [PATCH] mm: kvmalloc does not fallback to vmalloc for incompatible
gfp flags
kvmalloc warned about incompatible gfp_mask to catch abusers (mostly
GFP_NOFS) with an intention that this will motivate authors of the code
to fix those. Linus argues that this just motivates people to do even
more hacks like
if (gfp == GFP_KERNEL)
kvmalloc
else
kmalloc
I haven't seen this happening much (bucket_lock special cases an atomic
allocation) but it is true that we can grow those in future. Therefore
Linus suggested to simply not fallback to vmalloc for incompatible gfp
flags and rather stick with the kmalloc path.
Requested-by: Linus Torvalds [off-list ref]
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
lib/bucket_locks.c | 5 +----
mm/util.c | 6 ++++--
2 files changed, 5 insertions(+), 6 deletions(-)
@@ -30,10 +30,7 @@ int alloc_bucket_spinlocks(spinlock_t **locks, unsigned int *locks_mask,}if(sizeof(spinlock_t)!=0){-if(gfpflags_allow_blocking(gfp))-tlocks=kvmalloc(size*sizeof(spinlock_t),gfp);-else-tlocks=kmalloc_array(size,sizeof(spinlock_t),gfp);+tlocks=kvmalloc_array(size,sizeof(spinlock_t),gfp);if(!tlocks)return-ENOMEM;for(i=0;i<size;i++)
Now that we know that rhashtable_init() will not fail, we
can get rid of a lot of the unnecessary cleanup paths when
the call errored out.
Signed-off-by: Davidlohr Bueso <redacted>
---
ipc/msg.c | 9 ++++-----
ipc/namespace.c | 20 ++++----------------
ipc/sem.c | 10 ++++------
ipc/shm.c | 9 ++++-----
ipc/util.c | 18 +++++-------------
ipc/util.h | 18 +++++++++---------
6 files changed, 30 insertions(+), 54 deletions(-)
In sysvipc we have an ids->tables_initialized regarding the
rhashtable, introduced in:
0cfb6aee70b (ipc: optimize semget/shmget/msgget for lots of keys).
It's there, specifically, to prevent nil pointer dereferences,
from using an uninitialized api. Considering how rhashtable_init()
can fail (probably due to ENOMEM, if anything), this made the
overall ipc initialization capable of failure as well. That alone
is ugly, but fine, however I've spotted a few issues regarding the
semantics of tables_initialized (however unlikely they may be):
- There is inconsistency in what we return to userspace: ipc_addid()
returns ENOSPC which is certainly _wrong_, while ipc_obtain_object_idr()
returns EINVAL.
- After we started using rhashtables, ipc_findkey() can return nil upon
!tables_initialized, but the caller expects nil for when the ipc structure
isn't found, and can therefore call into ipcget() callbacks.
Now that rhashtable initialization cannot fail, we can properly
get rid of the hack altogether.
Signed-off-by: Davidlohr Bueso <redacted>
---
include/linux/ipc_namespace.h | 1 -
ipc/util.c | 23 ++++++++---------------
2 files changed, 8 insertions(+), 16 deletions(-)
I certainly can't complain about this small code removal, but I think if we
did the kvmalloc_node() cleanup, we'd be able to get rid of even more.
For example, bucket_table_alloc() does that
if (gfp != GFP_KERNEL)
tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
else
tbl = kvzalloc(size, gfp);
purely due to the kvalloc_node() oddity. Wouldn't it be nice to just write
it as
tbl = kvzalloc(size, gfp);
knowing that the whole point of all the kv*alloc*() functions is to "just
do the right thing given size, gpf mask, and ease of allocation".
Linus
I certainly can't complain about this small code removal, but I think if we
did the kvmalloc_node() cleanup, we'd be able to get rid of even more.
For example, bucket_table_alloc() does that
if (gfp != GFP_KERNEL)
tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
else
tbl = kvzalloc(size, gfp);
purely due to the kvalloc_node() oddity. Wouldn't it be nice to just write
it as
tbl = kvzalloc(size, gfp);
knowing that the whole point of all the kv*alloc*() functions is to "just
do the right thing given size, gpf mask, and ease of allocation".
Yes this makes a lot of sense. I'll see about adding it on top.
Thanks,
Davidlohr