Re: What parts of UMA are part of the stable ABI?
I've put the full patch to convert uma_malloc and uma_free to accept a vm_size_t up for review[1]. It ended up being more extensive than expected as a fair number of places do use uma_set_allocf(). I do plan on MFC'ing this patch. This survive a make tinderbox (ignoring some vt-related LINT kernel build failures) I haven't attempted converting the uma ctors, etc over to vm_size_t yet, but I do plan on doing that still. [1] https://reviews.freebsd.org/D2106 ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: What parts of UMA are part of the stable ABI?
On Wednesday, March 18, 2015 12:28:07 PM Adrian Chadd wrote: On 18 March 2015 at 08:23, John Baldwin j...@freebsd.org wrote: On Wednesday, March 18, 2015 11:19:21 AM Ryan Stone wrote: On Wed, Mar 18, 2015 at 10:24 AM, John Baldwin j...@freebsd.org wrote: I do think the normal zone callbacks passed to uma_zcreate() are too public to change. Or at least, you would need to do some crazy ABI shim where you have a uma_zcreate_new() that you map to uma_zcreate() via a #define for the API, but include a legacy uma_zcreate() symbol that older modules can call (and then somehow tag the old function pointers via an internal flag in the zone and patch UMA to cast to the old function signatures for zones with that flag). I really wasn't clear here. I definitely don't think that changing the ctor, etc to accept a size_t is MFC'able, and I don't think that the problem (which is really only theoretical at this point) warrants an MFC to -stable. I was talking about potentially doing it in a separate commit to head, but that does leave -stable and head with a different API. This can be painful for downstream consumers to deal with, which is why I wanted comments. I actually think the API change to fix the zone callbacks is fine to change in HEAD. I don't think that is too disruptive for folks who might be sharing code across branches (they can use a local typedef to work around it or some such). +1. This isn't exposed to userland, right? So I wouldn't worry about. Kernel progress can't be held back because we're afraid of kernel ABI changes that fix actual bugs. I think that's a bit too cavalier. Just because it fixes a bug doesn't mean we don't care about the ABI implications in general. (That is, I think your blanket implication that it's ok to break the kernel ABI at anytime if the change fixes a bug is wrong.) It is possible to fix bugs while preserving the ABI, it just requires more work. Our ABI constraints are not just for userland, we do also try to preserve the ABI of a subset of kernel symbols as well (mostly those used by device drivers). The stickler though is which symbols fall into that category since the line for the kernel is much fuzzier than for userland. In general I would err on the side of caution and provide shims if they are feasible. I do think that the uma_alloc case is obscure enough to not be one we care about for ABI compat. I couldn't find the bpf example I was thinking of earlier, but this commit includes the shim method I described previously to preserve the ABI for old modules while fixing the API for new ones (and this was to fix a bug): https://svnweb.freebsd.org/base?view=revisionrevision=196006 -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: What parts of UMA are part of the stable ABI?
On Friday, March 13, 2015 07:48:38 PM Ryan Stone wrote: In this freebsd-hackers thread[1], a user reported that 10.1-RELEASE crashes during boot on a system with 3TB of RAM. As it turns out, when you have that much RAM ZFS autotunes itself to allocate a 6GB hash table. This triggers a nasty 32-bit integer truncation bug in malloc(9). malloc() calls uma_large_malloc(), but uma_large_malloc() accepts an int instead of a size_t and all kinds of hilarity can ensure from there. The user has confirmed that the page in [2] fixed the kernel from instantly panicking once zfs.ko was loaded. I'm a bit concerned about whether the patch as written is an MFC candidate though. uma_large_malloc() calls page_alloc() to actuallly allocate the memory, and page_alloc() also accepts an int size parameter. This is where things get tricky. The signature for page_alloc() is governed by the uma_alloc() typedef, as uma also uses it internally for allocating memory for uma_zones. There is even a uma_zone_set_allocf() API for overriding the default allocation function. So there's definitely an argument to be made the the signature of page_alloc() being a part of the stable ABI. I have no hesitation in saying that uma_large_malloc() is not a stable API and changing it is fair game. If uma_alloc() is a part of the stable API, then it's simple enough to commit a 64-bit safe allocation function for uma_large_malloc() to call and changing page_alloc() to call it instead. That commit can be MFC'ed, and a follow-up commit could convert the UMA APIs to use size_t everywhere. I think uma_zone_set_allocf() is most likely obscure enough to not be used outside of the places in the kernel where it is used. From that, I think you are fine to change uma_alloc()'s signature. While I am at this, I'd like to also change the uma init/fini/ctor/dtor to also use size_t. I'm a little torn on this because this will definitely cause a lot of churn, both in the tree and for downstream consumers, and there's not necessarily going to be a big benefit to it. However, I suppose that the existence of machines where 4GB is less than 1% of system memory may mean that allocating 4GB at a time may not that outlandish. I can definitely be talked out of this though. I do think the normal zone callbacks passed to uma_zcreate() are too public to change. Or at least, you would need to do some crazy ABI shim where you have a uma_zcreate_new() that you map to uma_zcreate() via a #define for the API, but include a legacy uma_zcreate() symbol that older modules can call (and then somehow tag the old function pointers via an internal flag in the zone and patch UMA to cast to the old function signatures for zones with that flag). Note that if you are really paranoid you could do the same thing with uma_zone_set_allocf(). It would break the API, but would preserve the ABI (i.e. leave an existing uma_zone_set_allocf() function that accepts the old prototype but have a uma_zone_set_allocf_new() that gets mapped to uma_zone_set_allocf via a #define). -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: What parts of UMA are part of the stable ABI?
On Wed, Mar 18, 2015 at 10:24 AM, John Baldwin j...@freebsd.org wrote: I do think the normal zone callbacks passed to uma_zcreate() are too public to change. Or at least, you would need to do some crazy ABI shim where you have a uma_zcreate_new() that you map to uma_zcreate() via a #define for the API, but include a legacy uma_zcreate() symbol that older modules can call (and then somehow tag the old function pointers via an internal flag in the zone and patch UMA to cast to the old function signatures for zones with that flag). I really wasn't clear here. I definitely don't think that changing the ctor, etc to accept a size_t is MFC'able, and I don't think that the problem (which is really only theoretical at this point) warrants an MFC to -stable. I was talking about potentially doing it in a separate commit to head, but that does leave -stable and head with a different API. This can be painful for downstream consumers to deal with, which is why I wanted comments. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: What parts of UMA are part of the stable ABI?
On Wednesday, March 18, 2015 11:19:21 AM Ryan Stone wrote: On Wed, Mar 18, 2015 at 10:24 AM, John Baldwin j...@freebsd.org wrote: I do think the normal zone callbacks passed to uma_zcreate() are too public to change. Or at least, you would need to do some crazy ABI shim where you have a uma_zcreate_new() that you map to uma_zcreate() via a #define for the API, but include a legacy uma_zcreate() symbol that older modules can call (and then somehow tag the old function pointers via an internal flag in the zone and patch UMA to cast to the old function signatures for zones with that flag). I really wasn't clear here. I definitely don't think that changing the ctor, etc to accept a size_t is MFC'able, and I don't think that the problem (which is really only theoretical at this point) warrants an MFC to -stable. I was talking about potentially doing it in a separate commit to head, but that does leave -stable and head with a different API. This can be painful for downstream consumers to deal with, which is why I wanted comments. I actually think the API change to fix the zone callbacks is fine to change in HEAD. I don't think that is too disruptive for folks who might be sharing code across branches (they can use a local typedef to work around it or some such). -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: What parts of UMA are part of the stable ABI?
On 3/19/15 3:28 AM, Adrian Chadd wrote: On 18 March 2015 at 08:23, John Baldwin j...@freebsd.org wrote: On Wednesday, March 18, 2015 11:19:21 AM Ryan Stone wrote: On Wed, Mar 18, 2015 at 10:24 AM, John Baldwin j...@freebsd.org wrote: I do think the normal zone callbacks passed to uma_zcreate() are too public to change. Or at least, you would need to do some crazy ABI shim where you have a uma_zcreate_new() that you map to uma_zcreate() via a #define for the API, but include a legacy uma_zcreate() symbol that older modules can call (and then somehow tag the old function pointers via an internal flag in the zone and patch UMA to cast to the old function signatures for zones with that flag). I really wasn't clear here. I definitely don't think that changing the ctor, etc to accept a size_t is MFC'able, and I don't think that the problem (which is really only theoretical at this point) warrants an MFC to -stable. I was talking about potentially doing it in a separate commit to head, but that does leave -stable and head with a different API. This can be painful for downstream consumers to deal with, which is why I wanted comments. I actually think the API change to fix the zone callbacks is fine to change in HEAD. I don't think that is too disruptive for folks who might be sharing code across branches (they can use a local typedef to work around it or some such). +1. This isn't exposed to userland, right? So I wouldn't worry about. Kernel progress can't be held back because we're afraid of kernel ABI changes that fix actual bugs. I don't think we've ever aid we'd maintain kernel internal ABI across different code lines. We have said we'd try keep changes to some things easy to fix (e.g. network driver API) but a recompile is pretty much always needed. -adrian ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: What parts of UMA are part of the stable ABI?
[snip] So yes, I'd like to see this in -HEAD sooner rather than later. You did the great work of chasing it down, so let's get it in -HEAD. :) -a ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: What parts of UMA are part of the stable ABI?
On 18 March 2015 at 08:23, John Baldwin j...@freebsd.org wrote: On Wednesday, March 18, 2015 11:19:21 AM Ryan Stone wrote: On Wed, Mar 18, 2015 at 10:24 AM, John Baldwin j...@freebsd.org wrote: I do think the normal zone callbacks passed to uma_zcreate() are too public to change. Or at least, you would need to do some crazy ABI shim where you have a uma_zcreate_new() that you map to uma_zcreate() via a #define for the API, but include a legacy uma_zcreate() symbol that older modules can call (and then somehow tag the old function pointers via an internal flag in the zone and patch UMA to cast to the old function signatures for zones with that flag). I really wasn't clear here. I definitely don't think that changing the ctor, etc to accept a size_t is MFC'able, and I don't think that the problem (which is really only theoretical at this point) warrants an MFC to -stable. I was talking about potentially doing it in a separate commit to head, but that does leave -stable and head with a different API. This can be painful for downstream consumers to deal with, which is why I wanted comments. I actually think the API change to fix the zone callbacks is fine to change in HEAD. I don't think that is too disruptive for folks who might be sharing code across branches (they can use a local typedef to work around it or some such). +1. This isn't exposed to userland, right? So I wouldn't worry about. Kernel progress can't be held back because we're afraid of kernel ABI changes that fix actual bugs. -adrian ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
