| Message ID | 1424794259-30241-2-git-send-email-johunt@akamai.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 02/24/15 at 11:10am, Josh Hunt wrote: > If an rhashtable user defines a grow_decision fn they must also define a > max_shift parameter. > > Signed-off-by: Josh Hunt <johunt@akamai.com> Acked-by: Thomas Graf <tgraf@suug.ch> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Josh Hunt <johunt@akamai.com> Date: Tue, 24 Feb 2015 11:10:57 -0500 > If an rhashtable user defines a grow_decision fn they must also define a > max_shift parameter. > > Signed-off-by: Josh Hunt <johunt@akamai.com> I've already said today that I think this whole indirection stuff with grow and shrink decisions should simply go away. Everyone defines it to the generic rhashtable routine, therefore that should just be made private to lib/rhashtable.c, called directly, and the methods completely removed. Given that, this change makes no sense. When a limit is not specified, we should unconditionally grow rather than refuse to grow. One should not be required to specify this at all. If you have no idea what limit might be reasonable, you specify nothing at all and just let available memory be the limiting factor. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/24/2015 07:18 PM, David Miller wrote: ... > I've already said today that I think this whole indirection stuff > with grow and shrink decisions should simply go away. > > Everyone defines it to the generic rhashtable routine, therefore > that should just be made private to lib/rhashtable.c, called > directly, and the methods completely removed. > > Given that, this change makes no sense. > > When a limit is not specified, we should unconditionally grow rather > than refuse to grow. One should not be required to specify this at > all. If you have no idea what limit might be reasonable, you specify > nothing at all and just let available memory be the limiting factor. I agree. Fwiw, I believe this behavior came in as a regression via commit c0c09bfdc415 ("rhashtable: avoid unnecessary wakeup for worker queue"). Initially, if no max_shift was specified, we'd just expand further. I can take care of these above two fixups tomorrow, if you want. I presume you want to route both via -net, or just the growth limit issue via -net? I also have some optimizations I was working on last week for net-next, but I would wait for a -net into -net-next merge after that to avoid merge conflicts, if that's fine. ;) Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Daniel Borkmann <daniel@iogearbox.net> Date: Tue, 24 Feb 2015 20:48:10 +0100 > On 02/24/2015 07:18 PM, David Miller wrote: > ... >> I've already said today that I think this whole indirection stuff >> with grow and shrink decisions should simply go away. >> >> Everyone defines it to the generic rhashtable routine, therefore >> that should just be made private to lib/rhashtable.c, called >> directly, and the methods completely removed. >> >> Given that, this change makes no sense. >> >> When a limit is not specified, we should unconditionally grow rather >> than refuse to grow. One should not be required to specify this at >> all. If you have no idea what limit might be reasonable, you specify >> nothing at all and just let available memory be the limiting factor. > > I agree. > > Fwiw, I believe this behavior came in as a regression via commit > c0c09bfdc415 ("rhashtable: avoid unnecessary wakeup for worker > queue"). > Initially, if no max_shift was specified, we'd just expand further. > > I can take care of these above two fixups tomorrow, if you want. > I presume you want to route both via -net, or just the growth limit > issue via -net? Let's fix as much crap as we can in -net. I'm going to have to do a huge backport of all the rhashtables to -stable at some point too. > I also have some optimizations I was working on last week for > net-next, but I would wait for a -net into -net-next merge after > that to avoid merge conflicts, if that's fine. ;) Yes, good idea :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/24/2015 12:18 PM, David Miller wrote: > From: Josh Hunt <johunt@akamai.com> > Date: Tue, 24 Feb 2015 11:10:57 -0500 > >> If an rhashtable user defines a grow_decision fn they must also define a >> max_shift parameter. >> >> Signed-off-by: Josh Hunt <johunt@akamai.com> > > I've already said today that I think this whole indirection stuff > with grow and shrink decisions should simply go away. > > Everyone defines it to the generic rhashtable routine, therefore > that should just be made private to lib/rhashtable.c, called > directly, and the methods completely removed. > > Given that, this change makes no sense. > > When a limit is not specified, we should unconditionally grow rather > than refuse to grow. One should not be required to specify this at > all. If you have no idea what limit might be reasonable, you specify > nothing at all and just let available memory be the limiting factor. > I don't particularly care how this gets fixed at this point, just that it gets fixed. Right now nft hash sets can't expand b/c of this limitation. If we fix it by removing the max_shift requirement that works for me :) Josh -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/24/15 at 04:31pm, David Miller wrote: > From: Daniel Borkmann <daniel@iogearbox.net> > > Fwiw, I believe this behavior came in as a regression via commit > > c0c09bfdc415 ("rhashtable: avoid unnecessary wakeup for worker > > queue"). > > Initially, if no max_shift was specified, we'd just expand further. > > > > I can take care of these above two fixups tomorrow, if you want. Thanks! > > I presume you want to route both via -net, or just the growth limit > > issue via -net? > > Let's fix as much crap as we can in -net. I'm going to have to do > a huge backport of all the rhashtables to -stable at some point > too. All fixes except 2-3 should only affect the post per bucket lock era. The fixes tags should be correct but I'll double check. I can take care of the backports if you like. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Thomas Graf <tgraf@suug.ch> Date: Tue, 24 Feb 2015 22:49:15 +0000 > On 02/24/15 at 04:31pm, David Miller wrote: >> From: Daniel Borkmann <daniel@iogearbox.net> >> Let's fix as much crap as we can in -net. I'm going to have to do >> a huge backport of all the rhashtables to -stable at some point >> too. > > All fixes except 2-3 should only affect the post per bucket lock > era. The fixes tags should be correct but I'll double check. I can > take care of the backports if you like. There were several bugs that are only cured by the "per bucket lock era" version fo rhashtable. I only applied all of that work to net-next at the time because suspected we'd still have to hash all of that out. Therefore, all of the rhashtable stuff that went into 4.0-rc1 is going to need to be backported. I also really wish Herbert Xu didn't drop off the map on this work, we could really use his expertiece and suggestions right now. :-/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 9cc4c4a..7d6f539 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -1077,7 +1077,8 @@ int rhashtable_init(struct rhashtable *ht, struct rhashtable_params *params) size = HASH_DEFAULT_SIZE; if ((params->key_len && !params->hashfn) || - (!params->key_len && !params->obj_hashfn)) + (!params->key_len && !params->obj_hashfn) || + (params->grow_decision && !params->max_shift)) return -EINVAL; if (params->nulls_base && params->nulls_base < (1U << RHT_BASE_SHIFT))
If an rhashtable user defines a grow_decision fn they must also define a max_shift parameter. Signed-off-by: Josh Hunt <johunt@akamai.com> --- lib/rhashtable.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)