Re: semantics of rhashtable and sysvipc

On Thu, May 24, 2018 at 12:08 PM Davidlohr Bueso wrote: > However, after how about the resize being based on HASH_MIN_SIZE instead of > HASH_DEFAULT_SIZE? I think that sounds reasonable. We wouldn't expect this to ever happen in practice, and as you say, if it *does* happen,

Re: semantics of rhashtable and sysvipc

On Thu, May 24, 2018 at 12:08 PM Davidlohr Bueso wrote: > However, after how about the resize being based on HASH_MIN_SIZE instead of > HASH_DEFAULT_SIZE? I think that sounds reasonable. We wouldn't expect this to ever happen in practice, and as you say, if it *does* happen, the size of the

Re: semantics of rhashtable and sysvipc

On Thu, 24 May 2018, Linus Torvalds wrote: This doesn't seem to be taking 'param->min_size' into account. It was in that rounded_hashtable_size() does, however, after more thought I think we can do better by taking it much more into account. I'm not sure that matters, but right now, if you

Re: semantics of rhashtable and sysvipc

On Thu, 24 May 2018, Linus Torvalds wrote: This doesn't seem to be taking 'param->min_size' into account. It was in that rounded_hashtable_size() does, however, after more thought I think we can do better by taking it much more into account. I'm not sure that matters, but right now, if you

Re: semantics of rhashtable and sysvipc

On Thu, May 24, 2018 at 10:23 AM Davidlohr Bueso wrote: > tbl = bucket_table_alloc(ht, size, GFP_KERNEL); > - if (tbl == NULL) > - return -ENOMEM; > + if (unlikely(tbl == NULL)) { > + size = min(size, HASH_DEFAULT_SIZE) / 2; > +

Re: semantics of rhashtable and sysvipc

On Thu, May 24, 2018 at 10:23 AM Davidlohr Bueso wrote: > tbl = bucket_table_alloc(ht, size, GFP_KERNEL); > - if (tbl == NULL) > - return -ENOMEM; > + if (unlikely(tbl == NULL)) { > + size = min(size, HASH_DEFAULT_SIZE) / 2; > + > +

Re: semantics of rhashtable and sysvipc

On Wed, 23 May 2018, Linus Torvalds wrote: One option is to make rhashtable_alloc() shrink the allocation and try again if it fails, and then you *can* do __GFP_NOFAIL eventually. The below attempts to implements this, along with converting the EINVAL cases to WARN_ON(). I've refactored

Re: semantics of rhashtable and sysvipc

On Wed, 23 May 2018, Linus Torvalds wrote: One option is to make rhashtable_alloc() shrink the allocation and try again if it fails, and then you *can* do __GFP_NOFAIL eventually. The below attempts to implements this, along with converting the EINVAL cases to WARN_ON(). I've refactored

Re: semantics of rhashtable and sysvipc

On Wed, 23 May 2018, Linus Torvalds wrote: On Wed, May 23, 2018 at 11:47 AM Davidlohr Bueso wrote: Note that even if the allocation was guaranteed, there are still param validations and rhashtable_init() can return -EINVAL. So? It's not going to happen, because you're

Re: semantics of rhashtable and sysvipc

On Wed, 23 May 2018, Linus Torvalds wrote: On Wed, May 23, 2018 at 11:47 AM Davidlohr Bueso wrote: Note that even if the allocation was guaranteed, there are still param validations and rhashtable_init() can return -EINVAL. So? It's not going to happen, because you're not going to give

Re: semantics of rhashtable and sysvipc

On Wed, May 23, 2018 at 11:47 AM Davidlohr Bueso wrote: > Note that even if the allocation was guaranteed, there are still param validations > and rhashtable_init() can return -EINVAL. So? It's not going to happen, because you're not going to give garbage parameters. Why

Re: semantics of rhashtable and sysvipc

On Wed, May 23, 2018 at 11:47 AM Davidlohr Bueso wrote: > Note that even if the allocation was guaranteed, there are still param validations > and rhashtable_init() can return -EINVAL. So? It's not going to happen, because you're not going to give garbage parameters. Why would you add a

Re: semantics of rhashtable and sysvipc

On Wed, 23 May 2018, Linus Torvalds wrote: So I'm perfectly fine with getting rid of 'tables_initialized'. But no, not with a BUG_ON(). If you cannot guarantee that the allocation works (using __GFP_NOFAIL is ok, for example - but it only works with small allocations), then you need to handle

Re: semantics of rhashtable and sysvipc

On Wed, 23 May 2018, Linus Torvalds wrote: So I'm perfectly fine with getting rid of 'tables_initialized'. But no, not with a BUG_ON(). If you cannot guarantee that the allocation works (using __GFP_NOFAIL is ok, for example - but it only works with small allocations), then you need to handle

Re: semantics of rhashtable and sysvipc

On Wed, May 23, 2018 at 10:41 AM Davidlohr Bueso wrote: > The second alternative would be to add a BUG_ON() if the initialization fails > and we get rid of all the tables_initialized hack. I see absolutely no value in an early boot BUG_ON(). Either we know the allocation

Re: semantics of rhashtable and sysvipc

On Wed, May 23, 2018 at 10:41 AM Davidlohr Bueso wrote: > The second alternative would be to add a BUG_ON() if the initialization fails > and we get rid of all the tables_initialized hack. I see absolutely no value in an early boot BUG_ON(). Either we know the allocation cannot fail - which is

Re: semantics of rhashtable and sysvipc

On Wed, 23 May 2018, Davidlohr Bueso wrote: I see two possible fixes. I guess a third option would be to make the hashtable static, but I'm not against using rhashtables so I'm not really considering this.

Re: semantics of rhashtable and sysvipc

On Wed, 23 May 2018, Davidlohr Bueso wrote: I see two possible fixes. I guess a third option would be to make the hashtable static, but I'm not against using rhashtables so I'm not really considering this.

semantics of rhashtable and sysvipc

Hi, 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

semantics of rhashtable and sysvipc

Hi, 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