subr_bus: introduce device_set_descf() and modify allocation logic

[christos]

Summary

device_set_descf() is a printf-like version of device_set_desc().

Allocation code has been transferred from device_set_desc_internal() to
device_set_desc_copy() and device_set_descf() to avoid complicating
device_set_desc_internal(). The "copy" argument in
device_set_desc_internal() has been replaced with a flag which is set
when the description string has been allocated with M_BUS.

Sponsored by: The FreeBSD Foundation
MFC after: 2 weeks

[markj]

sys/kern/subr_bus.c

2055

To be compatible with device_set_desc_copy() in the case where memory allocation fails, just set buf = NULL and pass that.

[christos]

Address Mark's comments.

[markj]

I like it. But please wait for Warner to chime in.

I can help convert device_set_desc_copy() callers to use device_set_descf(). Many of them become a bit simpler.

[markj]

Oh, we should also update the device_set_desc manual page.

[christos]

In D43370#988453, @markj wrote:

I like it. But please wait for Warner to chime in.

I can help convert device_set_desc_copy() callers to use device_set_descf(). Many of them become a bit simpler.

I think it's best to do that in a single commit along with the sound modules.

[christos]

Update man page.

[imp]

what's a use case for this? I think I know, but I thought I'd ask to make sure.

sys/kern/subr_bus.c

2011

Yea, this should be __DECONST, but it was what was there before...

2044

why not int?

2048

If you initialize this to NULL you don't need to set it on error. vasprintf is guaranteed to not touch it unless it returns success.

2056

I think we should NOT call this if we failed to allocate the memory since that will clear any old description. Instead we should return ENOMEM if ret < 0.

2057

This can fail and clear the old value.
If strdup fails, don't call device_set_desc_intenral.
Out of memory not changing anything is more consistent with what's there now.
Though most of the time we're called from a sleepable context... except at startup though you will be in a huge world of hurt if this fails on startup...

[imp]

tl;dr: I like the idea, but there's minor details that I need to quibble over. Too bad the quibbles were wrong.

Hmmm... but it also looks like I misremembered the original behavior and misread the code. so ignore my first comments...

The other alternative is to always copy the string value. Then we don't need a new function at all. We can make the current one just accept varargs... Though I suppose that since it's tagged as printf we'll get format warnings because those strings come from tables, so maybe we do need a new function after all.... If we really wanted to do this, we could do it with macro magic though, at the cost of more symbols being exposed... jhb? Is it worth it?

The name ending in 'f' isn't typical. That's usually for 'flags'. There's one other place that uses 'f' for format. There's no other examples, though, of modern apis that do anything: either they've always taken the extra args, or they are ancient and prepend 'v' like vprintf, which isn't quite the same thing... So the name is fine.

adding jhb@ to see if he thinks it would be a cleaner interface to do this, or the varargs trick with the same name. I ticked accept because I think I'm good either way.

sys/kern/subr_bus.c

2044

mirrors device_set_desc* that's already there.

2045

why do we need a new function here at all? Though I suppose it's ha

2056

But this mirrors the old behavior for desc_copy....

I Think it's a botch in the interface, but everybody assumes they can set the description. copy was added as an afterthought, but didn't really provide a good error signal... So I'm not sure if we should try to fix that with this interface, or continue the old interface...

Realistically, nobody is going to do anything with this signal, except maybe fail the device_attach. And if not here, then it will likely fail elsewhere (most drivers allocate a other stuff, and if a few strings are too much, then we're already in a lot of trouble, even for the kld case).

2057

oh, that is the old behavior. n/m. This is fine.

[markj]

sys/kern/subr_bus.c

2056

Yes, I don't see much reason to support returning an error.

Almost all drivers call this function during probe, not attach, and I think almost all of them could use M_WAITOK. asprintf(9) hard-codes M_NOWAIT, but we could add a asprintf_flags() which takes malloc wait flags as a parameter, mirroring strdup_flags().

[christos]

Address Warner's comments:

• Use __DECONST() in device_set_desc_internal().
• Initialize buffer to NULL in device_set_descf() to get rid of the if-statement in case vasprintf() fails.

[jhb]

I would support making device_set_desc() just be device_set_descf() that always does a copy. Though you'd probably have to fix some places that do table lookups to use "%s" to format the string from the table if you did that.

sys/kern/subr_bus.c

2000

I suggest s/alloced/allocated/

2040

Agree with Mark on using a M_WAITOK version of asprintf here.

2056

I concur with Mark's suggestion of just using M_WAITOK here. Most of subr_bus.c can actually use M_WAITOK which would remove various unhandled edge cases.

[imp]

So there's approx 1380 calls to device_set_desc today.
1050 of these occur on a line with " in it. We can assume

of the 330 that remain:
~30 have split lines
~40 have upper case and are #define sourced (eg #define FOO "Foominator" ... device_set_desc(dev, FOO))
About ~160 definitely use a table lookup and pass the results to device_set_desc
The other ~100 or so look like they could benefit from this new construct. Though there's a few that have a big case statement with desc = in each one and then there's one call at the end so maybe not all these benefit.

So that's a lot of work to not do something clever as a transition aide for calls with just 2 arg esp for MFCability... but it's also a lot of work to roll this out to places that can use it to simplify things...

So I'm not sure what these numbers tell me.

[imp]

An AMD64 generic kernel and modules generates 242 error files if I change device_set_desc to be varadic with the printflike markings.
so this is like 120 files needing love, which is roughly in line with the 160 from the other analysis.

[christos]

In D43370#988944, @imp wrote:

So that's a lot of work to not do something clever as a transition aide for calls with just 2 arg esp for MFCability... but it's also a lot of work to roll this out to places that can use it to simplify things...

I think some of the first modules that can initially benefit from device_set_descf() which do not require that much work are the audio drivers. By the looks of it, it might be too much (perhaps unnecessary) work to do this for every single module that could benefit from this function.

[christos]

sys/kern/subr_bus.c

2040

So we need both vasprintf_flags() and asprintf_flags(). Then vasprintf() will call vasprintf_flags(M_NOWAIT) and asprintf() will call asprintf_flags(M_NOWAIT) respectively, right?

[markj]

So let's try converting some drivers to use this function and see what the diff looks like? The sound drivers seem like a good place to start.

sys/kern/subr_bus.c

2040

Yes, we'd need to add those functions, since asprintf() currently hard-codes M_NOWAIT.

But I would do that in a separate commit, after evaluating existing device_set_desc*() callers and seeing exactly how many might require M_NOWAIT after all.

[christos]

In D43370#989267, @markj wrote:

So let's try converting some drivers to use this function and see what the diff looks like? The sound drivers seem like a good place to start.

Here. This is without the use of device_set_descf() in uaudio.
https://reviews.freebsd.org/P623

[markj]

In D43370#989319, @christos wrote:

In D43370#989267, @markj wrote:

So let's try converting some drivers to use this function and see what the diff looks like? The sound drivers seem like a good place to start.

Here. This is without the use of device_set_descf() in uaudio.
https://reviews.freebsd.org/P623

Nice, I think that's a good simplification. @imp @jhb how about we proceed with the interface as currently proposed, and deal with M_NOWAIT/M_WAITOK considerations and potential unification with device_set_desc() further down the road?

[jhb]

In D43370#989324, @markj wrote:

In D43370#989319, @christos wrote:

In D43370#989267, @markj wrote:

So let's try converting some drivers to use this function and see what the diff looks like? The sound drivers seem like a good place to start.

Here. This is without the use of device_set_descf() in uaudio.
https://reviews.freebsd.org/P623

Nice, I think that's a good simplification. @imp @jhb how about we proceed with the interface as currently proposed, and deal with M_NOWAIT/M_WAITOK considerations and potential unification with device_set_desc() further down the road?

I think we can certainly defer unifying the APIs given it looks pretty invasive.

I think by definition device_set_desc is always safe to use M_WAITOK btw. device_probe is generally run from device_probe_and_attach and device_attach generally speaking can only run in sleepable contexts. Drivers that can add new devices at runtime via hot plug defer adding and removing devices to a taskqueue because there's just too much stuff in attach/detach in general that requires sleeping. device_probe ends up in the same boat, so it really should just be safe to always use M_WAITOK for device_*_desc. That said, the M_WAITOK fixes are also orthogonal. I would at least fix the one explicit M_NOWAIT in the current patch to be M_WAITOK since it's easy to do so now. The asprintf() thing can be fixed either before or after this patch IMO.

[markj]

In D43370#989565, @jhb wrote:

In D43370#989324, @markj wrote:

In D43370#989319, @christos wrote:

In D43370#989267, @markj wrote:

So let's try converting some drivers to use this function and see what the diff looks like? The sound drivers seem like a good place to start.

Here. This is without the use of device_set_descf() in uaudio.
https://reviews.freebsd.org/P623

Nice, I think that's a good simplification. @imp @jhb how about we proceed with the interface as currently proposed, and deal with M_NOWAIT/M_WAITOK considerations and potential unification with device_set_desc() further down the road?

I think we can certainly defer unifying the APIs given it looks pretty invasive.

I think by definition device_set_desc is always safe to use M_WAITOK btw. device_probe is generally run from device_probe_and_attach and device_attach generally speaking can only run in sleepable contexts. Drivers that can add new devices at runtime via hot plug defer adding and removing devices to a taskqueue because there's just too much stuff in attach/detach in general that requires sleeping. device_probe ends up in the same boat, so it really should just be safe to always use M_WAITOK for device_*_desc. That said, the M_WAITOK fixes are also orthogonal. I would at least fix the one explicit M_NOWAIT in the current patch to be M_WAITOK since it's easy to do so now. The asprintf() thing can be fixed either before or after this patch IMO.

The one explicit M_NOWAIT in the current patch is carried over from the existing implementation of device_set_desc_copy(). Until all of the callers are audited (which can be done while they're being converted to use device_set_descf()), I would avoid changing the malloc() behaviour. I went through maybe 40 callers just now and agree that it's most likely a safe change, but it's also orthogonal to the patch as proposed.

[imp]

I agree with John more than Mark: Even though it the NOWAIT was copied, it's completely safe to be WAIT. All probe/attach routines have always run in a sleepable context, and will be in the future. Especially for memory allocation. Though the initial probe/attach can't do much if we can't allocate memory (though the system can't do much if we can't allocate enough memory to get through boot).

But I won't insist that the new thing not use WAITOK. Seems like we shouldn't be adding cases we need to fix later though. We have way too many of those in CAM, but CAM has a much much more complex history.

[imp]

Actually, the more I think about it, I'll make a followup with the NOWAIT -> WAITOK change...

[christos]

In D43370#989610, @imp wrote:

Actually, the more I think about it, I'll make a followup with the NOWAIT -> WAITOK change...

Should I go ahead with the patch and let you take care of the M_WAITOK changes?

[imp]

Yes. Go ahead