Paths

Table of Contentst

-
sys/dev/sound/pcm/
-
dev/
-
sound/
-
pcm/
-
feeder.h
2/5
feeder_eq.c
2/5
feeder_format.c
2/5
feeder_matrix.c
-
feeder_mixer.c
-
feeder_rate.c
-
feeder_volume.c
-
pcm.h
-
sndstat.c
-
sound.h
-
tests/sys/sound/
-
sys/
-
sound/
-
pcm_read_write.c

sound: Refactor the format conversion framework
ClosedPublic
Actions

Authored by christos on Dec 5 2024, 5:57 PM.

Details

Reviewers

markj
emaste
dev_submerge.ch

Commits

rG33529d6ad44d: sound: Refactor the format conversion framework
rG433e270f341c: sound: Refactor the format conversion framework

Summary

Merge the PCM_READ|WRITE_* macros defined in pcm/pcm.h, as well as the
intpcm_read|write_* macros defined in pcm/feeder_format.c, into six
inline functions: pcm_sample_read|write[_norm|calc](). The absence of
macro magic makes the code significantly easier to read, use and modify.

Since these functions take the input/output format as a parameter, get
rid of the read() and write() function pointers defined in struct
feed_format_info, as well as the feeder_format_read|write_op()
functions, and use the new read/write functions directly.

Sponsored by: The FreeBSD Fondation
MFC after: 1 week

Test Plan

Apply D48330 before this patch and run the tests.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 61755
Build 58639: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Remove unit test in favor of D48330. Work will continue there. Depend on it.
Make shift functions inline, instead of macros.
Mark read/write functions as __unused to suppress compiler warnings about unused shift functions in various files.
Fix regressions with unsigned format conversion. Now passes D48330 tests.
Modify D48330 to work with the refactor.

BTW, I hope you all had a pleasant start into 2025!

Happy new year! :-)

Harbormaster completed remote builds in B61561: Diff 148897.Jan 7 2025, 5:49 PM

christos edited the summary of this revision. (Show Details)Jan 7 2025, 5:51 PM

christos edited the test plan for this revision. (Show Details)

christos added a child revision: D48421: sound: Turn clamp macros into a function.Jan 10 2025, 6:36 PM

dev_submerge.ch mentioned this in D48421: sound: Turn clamp macros into a function.Jan 12 2025, 1:36 AM

There's actually three different sample value types involved here:

Original magnitude of the sample format (_PCM_READ_*() and _PCM_WRITE_*()).
Original magnitude, shift to 24 bit for 32 bit processing (PCM_READ_*() and PCM_WRITE_*()).
Normalized to 32 bit magnitude (intpcm_read_*() and intpcm_write_*()).

The refactoring is currently missing the first, I suggest that pcm_sample_read() and pcm_sample_write() should implement that instead of the second (see inline comments). We can handle the second type in a separate function. The third type then becomes pcm_sample_read_shift() and pcm_sample_write_shift().

sys/dev/sound/pcm/pcm.h
227–230	This should be factored out into a separate function.
247–250	This should be factored out into a separate function.

christos added a comment.Jan 12 2025, 8:31 PM

This comment was removed by christos.

christos added a comment.Jan 12 2025, 8:38 PM

This comment was removed by christos.

christos added a comment.Jan 12 2025, 8:46 PM

This comment was removed by christos.

Scratch that. I remembered the reason for the shift down to 24-bits is that if we are on a 32-bit machine, then can save some space this way and avoid overflows...

christos marked 2 inline comments as done.Jan 12 2025, 9:39 PM

christos added inline comments.

sys/dev/sound/pcm/pcm.h
247–250	The `PCM_WRITE_*` are unused, so I think we can get away with not implementing this. No?

Depend on D48077. The test won't compile without this patch.
Introduce pcm_sample_read_24bit() and use in place of PCM_READ_*. I did not implement pcm_sample_write_24bit() respectively, since PCM_WRITE_* was never used.

Harbormaster completed remote builds in B61683: Diff 149158.Jan 12 2025, 9:50 PM

christos added a parent revision: D48077: include: add a userland version of __assert_unreachable.Jan 12 2025, 9:51 PM

christos mentioned this in D48394: sound: Simplify pcm/feeder_mixer.c.Jan 12 2025, 9:54 PM

dev_submerge.ch added inline comments.Jan 12 2025, 11:43 PM

sys/dev/sound/pcm/pcm.h
75–77	I don't think we can remove this (yet)?
247–250	It's not strictly necessary to implement it, but it's only unused because the original code includes the shift back to 32bit in the clamp macro which is ... at least non-obvious. If we want to untangle that convolution (I would), there will be a need to do the shift to 32bit outside of the clamp function. My idea was actually to have this 32bit <-> 24bit transformation (or no-op) in a separate function without calling `pcm_sample_write()` or `pcm_sample_read()`, but never mind. What is important is to make it obvious what sample magnitude each feeder code is dealing with.

dev_submerge.ch added inline comments.Jan 13 2025, 12:00 AM

sys/dev/sound/pcm/pcm.h
147	As originally suggested by @markj in D48330.
223
229

christos edited the summary of this revision. (Show Details)Jan 13 2025, 12:19 AM

christos edited the test plan for this revision. (Show Details)

Address comments.

Harbormaster completed remote builds in B61686: Diff 149162.Jan 13 2025, 12:20 AM

christos added inline comments.Jan 13 2025, 12:20 AM

sys/dev/sound/pcm/pcm.h
75–77	This was a leftover from an experimental change. The diff is fixed already, but I didn't want to keep spamming everyone with more updates. :-)
247–250	I will implement the function, but for now I will keep it unused, since it'd require to change the logic of the clamp function as well in order to use `pcm_sample_write_24bit()` and not do the shift in the clamp function. I think it's better to implement this in a followup patch to not over-complicate this one.

Use pcm_sample_write_24bit() instead of calling PCM_CLAMP_* and then
passing the result to pcm_sample_write(). I said I would do it in a separate
patch, but I think it's not that bad to have it here.

Harbormaster completed remote builds in B61755: Diff 149329.Jan 16 2025, 1:13 PM

christos added inline comments.Jan 16 2025, 1:16 PM

sys/dev/sound/pcm/feeder_mixer.c
68	We are discussing this in D48394 as well, but we lose precision here, and in the other call of `pcm_sample_write_24bit()`. If this function is called with a 32-bit format, then the type assigned to `z` will be `int64_t` if `SND_PCM_64` is set. `pcm_sample_write_*()` take the sample as `intpcm_t`, aka `int32_t`. I think we need to address this `intpcm_t` thing ASAP. It's quite confusing.

Include assert.h if we're not in the kernel, and also set v to in
pcm_sample_read() default case, so that the compiler doesn't complain
when building the tests.

Harbormaster completed remote builds in B61758: Diff 149332.Jan 16 2025, 1:33 PM

christos edited the summary of this revision. (Show Details)Jan 21 2025, 11:30 AM

Rename *_shift to *_norm and *_24bit to *_calc, similar to D48330.
Adapt D48330. Tests pass.

Harbormaster completed remote builds in B61859: Diff 149652.Jan 21 2025, 11:31 AM

christos mentioned this in D48036: sound: Remove feed_matrix_apply_generic().Jan 21 2025, 11:38 AM

@dev_submerge.ch Both 32 and 64 bit tests pass normally (with the fix mentioned in D48330). Are we done with this one?

In D47932#1110250, @christos wrote:

@dev_submerge.ch Both 32 and 64 bit tests pass normally (with the fix mentioned in D48330). Are we done with this one?

Nope. First we need to know whether the unit test is correct and passes on all architectures, AFAICT we only have little endian results yet.

dev_submerge.ch added inline comments.Jan 26 2025, 5:48 PM

sys/dev/sound/pcm/feeder_volume.c
68–71	Is there a reason you did omit the `x = PCM_CLAMP_##SIGN##BIT(v);` here? We might run into overflows without it, no?

christos added inline comments.Jan 27 2025, 11:18 AM

sys/dev/sound/pcm/feeder_volume.c
68–71	I might be wrong, but I used the `_calc` variant intentionally, so that we can also clamp 32-bit samples when `SND_PCM_64` is not set. Since we are always working with `int32_t`s essentially, I'm not sure how we'd end up with an overflow in the <32-bit cases.

dev_submerge.ch added inline comments.Jan 27 2025, 4:07 PM

sys/dev/sound/pcm/feeder_volume.c
68–71	The `_calc` variants do not clamp. You need an extra function for that. As far as I understand, `FEEDVOLUME_CALC##BIT(x, vol[matrix[i]])` may boost the volume and thus generate values that overflow the bit resolution of the sample format here. If we just write values as is, we'll get truncated noise samples instead of just clipping with clamped values.

Address comments.

Harbormaster completed remote builds in B62143: Diff 150191.Jan 30 2025, 3:32 PM

I think with the recent fixes to the tests, and since all comments here have been addressed, we could go ahead with this?

markj added inline comments.Feb 11 2025, 3:12 PM

sys/dev/sound/pcm/pcm.h
146	What's with the `__unused` annotations? Is it to quiet warnings about unused functions when this header is included? I would expect `__always_inline` to be sufficient.

christos added inline comments.Feb 11 2025, 3:17 PM

sys/dev/sound/pcm/pcm.h

146

Indeed I added __unused to silence warnings; just __always_inline does not seem to be enough.

In file included from /mnt/src/sys/dev/sound/pcm/feeder_rate.c:58:
/mnt/src/sys/dev/sound/pcm/pcm.h:225:1: warning: unused function 'pcm_sample_read_norm' [-Wunused-function]
  225 | pcm_sample_read_norm(const uint8_t *src, uint32_t fmt)
      | ^~~~~~~~~~~~~~~~~~~~
/mnt/src/sys/dev/sound/pcm/pcm.h:234:1: warning: unused function 'pcm_sample_read_calc' [-Wunused-function]
  234 | pcm_sample_read_calc(const uint8_t *src, uint32_t fmt)
      | ^~~~~~~~~~~~~~~~~~~~
/mnt/src/sys/dev/sound/pcm/pcm.h:366:1: warning: unused function 'pcm_sample_write_norm' [-Wunused-function]
  366 | pcm_sample_write_norm(uint8_t *dst, intpcm_t v, uint32_t fmt)
      | ^~~~~~~~~~~~~~~~~~~~~
/mnt/src/sys/dev/sound/pcm/pcm.h:375:1: warning: unused function 'pcm_sample_write_calc' [-Wunused-function]
  375 | pcm_sample_write_calc(uint8_t *dst, intpcm_t v, uint32_t fmt)
      | ^~~~~~~~~~~~~~~~~~~~~
4 warnings generated.

That's just a sample. There are more warnings.

markj added inline comments.Feb 11 2025, 3:20 PM

sys/dev/sound/pcm/pcm.h
146	What happens if you add `__inline` after `__always_inline`?

christos added inline comments.Feb 11 2025, 3:24 PM

sys/dev/sound/pcm/pcm.h
146	That works. Should we go with this instead of `__unused`?

markj added inline comments.Feb 11 2025, 3:25 PM

sys/dev/sound/pcm/pcm.h
146	Yes please.

Use __inline instead __unused to silence warnings.

Harbormaster completed remote builds in B62371: Diff 150846.Feb 11 2025, 3:27 PM

Remove __always_inline. Related discussion in D47638.

Harbormaster completed remote builds in B62444: Diff 151036.Feb 14 2025, 3:55 PM

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

In D47932#1117964, @dev_submerge.ch wrote:

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

I also do not completely understand why the warnings go away with an additional __inline, to be honest. I trust @markj's judgement on this.

In D47932#1118261, @christos wrote:

In D47932#1117964, @dev_submerge.ch wrote:

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

I also do not completely understand why the warnings go away with an additional __inline, to be honest. I trust @markj's judgement on this.

IIRC the additional __inline is what broke compilation with it being a duplicate declaration specifier. But instead of reverting that and fixing the original unused warnings, we're now relaxing the inline specifier to something which has a very compiler-specific and C version dependent interpretation. Not to doubt Mark's judgement, but I'm not convinced this is a good solution for the underlying problem. We still have unused function definitions in the translation units.

In D47932#1118466, @dev_submerge.ch wrote:

In D47932#1118261, @christos wrote:

In D47932#1117964, @dev_submerge.ch wrote:

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

I also do not completely understand why the warnings go away with an additional __inline, to be honest. I trust @markj's judgement on this.

IIRC the additional __inline is what broke compilation with it being a duplicate declaration specifier. But instead of reverting that and fixing the original unused warnings, we're now relaxing the inline specifier to something which has a very compiler-specific and C version dependent interpretation.

My complaint about the original patch is that the __unused function is misleading, as the functions are not unused. It's very common for us to define static functions in header files, and they're virtually all defined __inline precisely to suppress warnings about unused functions, rather than out of any particular concern about controlling whether or not they do get inlined. So, yes, this is an abuse of the specifier, but all else being equal I'd rather handle that problem consistently within the tree.

Not to doubt Mark's judgement, but I'm not convinced this is a good solution for the underlying problem. We still have unused function definitions in the translation units.

In the absence of a good solution that's known to work with both clang and gcc, I withdraw my objection to using __always_inline+__unused, but I'm still a bit skeptical that that's a good idea. A compiler might well warn about not being able to honour __always_inline, and if pcm.h is ever installed to /usr/include (I assumed it already was, but apparently not), we won't control the compiler, so might easily see new problems.

Another solution would be to replace the __inline annotations with PCM_INLINE, and add #define PCM_INLINE __always_inline in compilation units where you really care about it, and let it default to __inline otherwise. But, since I'm not contributing to this area, I won't insist on anything in particular.

In D47932#1118481, @markj wrote:

In D47932#1118466, @dev_submerge.ch wrote:

In D47932#1118261, @christos wrote:

In D47932#1117964, @dev_submerge.ch wrote:

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

I also do not completely understand why the warnings go away with an additional __inline, to be honest. I trust @markj's judgement on this.

IIRC the additional __inline is what broke compilation with it being a duplicate declaration specifier. But instead of reverting that and fixing the original unused warnings, we're now relaxing the inline specifier to something which has a very compiler-specific and C version dependent interpretation.

My complaint about the original patch is that the __unused function is misleading, as the functions are not unused. It's very common for us to define static functions in header files, and they're virtually all defined __inline precisely to suppress warnings about unused functions, rather than out of any particular concern about controlling whether or not they do get inlined. So, yes, this is an abuse of the specifier, but all else being equal I'd rather handle that problem consistently within the tree.

Ok, I see now where this is coming from. But since that abuse of __inline does not work together with the always inline that we want here, to me it seems like we're fixing the wrong end.

Not to doubt Mark's judgement, but I'm not convinced this is a good solution for the underlying problem. We still have unused function definitions in the translation units.

In the absence of a good solution that's known to work with both clang and gcc, I withdraw my objection to using __always_inline+__unused, but I'm still a bit skeptical that that's a good idea. A compiler might well warn about not being able to honour __always_inline, and if pcm.h is ever installed to /usr/include (I assumed it already was, but apparently not), we won't control the compiler, so might easily see new problems.

Another solution would be to replace the __inline annotations with PCM_INLINE, and add #define PCM_INLINE __always_inline in compilation units where you really care about it, and let it default to __inline otherwise. But, since I'm not contributing to this area, I won't insist on anything in particular.

The alternatives I see are

Avoid the definition of unused functions, through separate headers for example. I suspect we'll reduce the number of different read / write functions anyway.
Make these functions extern instead of static, implement them in one translation unit.
Different workarounds with compiler directions or flags.

Unless we're pressed to, I would really want to instruct the compiler to inline these functions whenever possible.

In D47932#1118510, @dev_submerge.ch wrote:

The alternatives I see are

Avoid the definition of unused functions, through separate headers for example. I suspect we'll reduce the number of different read / write functions anyway.

Make these functions extern instead of static, implement them in one translation unit.

Different workarounds with compiler directions or flags.

Unless we're pressed to, I would really want to instruct the compiler to inline these functions whenever possible.

I am voting for the second approach, seems like the most straight-forward solution.

Declare as extern in header file and define as __always_inline. Not sure if
that give us the desired functionality yet.

Harbormaster completed remote builds in B62576: Diff 151314.Feb 21 2025, 9:01 PM

In D47932#1119492, @christos wrote:

Declare as extern in header file and define as __always_inline. Not sure if
that give us the desired functionality yet.

Apparently my comment following this never went through and I saw it now. But as is expected, the tests do not build if we define the functions like this.

In D47932#1120121, @christos wrote:

In D47932#1119492, @christos wrote:

Declare as extern in header file and define as __always_inline. Not sure if
that give us the desired functionality yet.

Apparently my comment following this never went through and I saw it now. But as is expected, the tests do not build if we define the functions like this.

It looks like you moved the (one and only) implementation of the pcm_*() functions into feeder_format.c? That's not what I meant, this would prevent any inlining, completely. For a simpler solution, why don't you start with the previous approach where you stumbled upon the "unused" warnings, move the pcm_*() functions into separate headers (one for each pair), and then just include where we really use them?
I think that would even make good sense of the "unused" warnings as they are intended to work.

Also I'm sorry I can't contribute more often currently, I just started a new job and my spare time is really spread thin these days.

In D47932#1120403, @dev_submerge.ch wrote:

In D47932#1120121, @christos wrote:

In D47932#1119492, @christos wrote:

Declare as extern in header file and define as __always_inline. Not sure if
that give us the desired functionality yet.

Apparently my comment following this never went through and I saw it now. But as is expected, the tests do not build if we define the functions like this.

It looks like you moved the (one and only) implementation of the pcm_*() functions into feeder_format.c? That's not what I meant, this would prevent any inlining, completely.

I know. I only posted this as a WIP initially.

For a simpler solution, why don't you start with the previous approach where you stumbled upon the "unused" warnings, move the pcm_*() functions into separate headers (one for each pair), and then just include where we really use them?
I think that would even make good sense of the "unused" warnings as they are intended to work.

The only reason I didn't do this already is because of tidiness, since we'll introduce 2 more headers for 4 functions, although I'm not sure whether that's a good enough reason not to do it. Personally I wouldn't mind leaving them all in pcm.h with __unused, but if it's best to avoid it, then we can look into this approach. @markj what do you think?

Also I'm sorry I can't contribute more often currently, I just started a new job and my spare time is really spread thin these days.

No worries, I appreciate that you give part of your free time to review all my never-ending patches. Good luck with your new job. :-)

christos removed parent revisions: D48077: include: add a userland version of __assert_unreachable, D48330: sound: Unit test the pcm sample read and write macros.Feb 25 2025, 12:11 PM

christos added a child revision: D48032: sound: Remove macro magic from pcm/feeder_eq.c.Feb 25 2025, 12:33 PM

I think we should just go ahead with using __always_inline __unused for now.

Return to __always_inline __unused. I think it is better to leave it like
that for now and avoid complicating the code base further.

Harbormaster completed remote builds in B62785: Diff 151954.Mar 6 2025, 1:06 PM

markj accepted this revision.Mar 7 2025, 10:50 PM

This revision is now accepted and ready to land.Mar 7 2025, 10:50 PM

Closed by commit rG433e270f341c: sound: Refactor the format conversion framework (authored by christos). · Explain WhyMar 10 2025, 8:22 PM

This revision was automatically updated to reflect the committed changes.

christos added a commit: rG433e270f341c: sound: Refactor the format conversion framework.

christos removed a child revision: D47638: sound: Implement AFMT_FLOAT support.Mar 10 2025, 8:54 PM

christos added a commit: rG33529d6ad44d: sound: Refactor the format conversion framework.Mar 17 2025, 6:37 PM

Revision Contents
Changeset List

Path

Size

sys/

dev/

sound/

pcm/

7 lines

12 lines

221 lines

43 lines

10 lines

18 lines

7 lines

546 lines

1 line

1 line

tests/

sys/

sound/

pcm_read_write.c

154 lines

Diff 149329

View Options

sys/dev/sound/pcm/feeder.h

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines	for (n = 0; n < G_N_EVENTREFS; n++) {
break;		break;
}		}
}		}
if (TAILQ_EMPTY(&g_events))		if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events);		wakeup(&g_pending_events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
}		}

static int		struct g_event *
g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)		g_alloc_event(int flag)
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		impAuthorUnsubmitted Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). That's a good idea. imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of `g_vfs_done`: if (destroy) g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL); That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?) Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted. jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{		{
struct g_event *ep;		KASSERT(flag == M_WAITOK \|\| flag == M_NOWAIT,
		("Wrong flag to g_alloc_event"));

		return (g_malloc(sizeof(struct g_event), flag \| M_ZERO));
		}

		static void
		g_post_event_ep_va(g_event_t func, void arg, int wuflag,
		struct g_event *ep, va_list ap)
		{
void *p;		void *p;
u_int n;		u_int n;

g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag \| M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag;		ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) {		for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *);		p = va_arg(ap, void *);
if (p == NULL)		if (p == NULL)
break;		break;
g_trace(G_T_TOPOLOGY, " ref %p", p);		g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p;		ep->ref[n] = p;
}		}
KASSERT(p == NULL, ("Too many references to event"));		KASSERT(p == NULL, ("Too many references to event"));
ep->func = func;		ep->func = func;
ep->arg = arg;		ep->arg = arg;
mtx_lock(&g_eventlock);		mtx_lock(&g_eventlock);
TSHOLD("GEOM events");		TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events);		TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
wakeup(&g_wait_event);		wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags \|= TDP_GEOM;		curthread->td_pflags \|= TDP_GEOM;
thread_lock(curthread);		thread_lock(curthread);
curthread->td_flags \|= TDF_ASTPENDING;		curthread->td_flags \|= TDF_ASTPENDING;
thread_unlock(curthread);		thread_unlock(curthread);
		}

		void
		g_post_event_ep(g_event_t func, void arg, struct g_event *ep, ...)
		jhbUnsubmitted Not Done Inline Actions Why did you expose `wuflag` in this API? I would perhaps instead just have a new `g_waitfor_event_storage` if someone needs the `EV_WAKEUP` variant so that it mirrors the `g_post_event`/`g_waitfor_event` API. The 'waitfor' variant could be added in the future when it is needed. Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use `M_WAITOK` to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses `M_WAITOK`. jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
		impAuthorUnsubmitted Done Inline Actions I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense. imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
		{
		va_list ap;

		va_start(ap, ep);
		g_post_event_ep_va(func, arg, 0, ep, ap);
		va_end(ap);
		}


		static int
		g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)
		{
		struct g_event *ep;

		g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
		func, arg, flag, wuflag);
		KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
		("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
		ep = g_alloc_event(flag);
		if (ep == NULL)
		return (ENOMEM);
		if (epp != NULL)
		*epp = ep;
		g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0);		return (0);
}		}

int		int
g_post_event(g_event_t func, void arg, int flag, ...)		g_post_event(g_event_t func, void arg, int flag, ...)
{		{
va_list ap;		va_list ap;
int i;		int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines

View Options

sys/dev/sound/pcm/feeder_eq.c

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines	for (n = 0; n < G_N_EVENTREFS; n++) {
break;		break;
}		}
}		}
if (TAILQ_EMPTY(&g_events))		if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events);		wakeup(&g_pending_events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
}		}

static int		struct g_event *
g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)		g_alloc_event(int flag)
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		impAuthorUnsubmitted Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). That's a good idea. imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of `g_vfs_done`: if (destroy) g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL); That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?) Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted. jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{		{
struct g_event *ep;		KASSERT(flag == M_WAITOK \|\| flag == M_NOWAIT,
		("Wrong flag to g_alloc_event"));

		return (g_malloc(sizeof(struct g_event), flag \| M_ZERO));
		}

		static void
		g_post_event_ep_va(g_event_t func, void arg, int wuflag,
		struct g_event *ep, va_list ap)
		{
void *p;		void *p;
u_int n;		u_int n;

g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag \| M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag;		ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) {		for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *);		p = va_arg(ap, void *);
if (p == NULL)		if (p == NULL)
break;		break;
g_trace(G_T_TOPOLOGY, " ref %p", p);		g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p;		ep->ref[n] = p;
}		}
KASSERT(p == NULL, ("Too many references to event"));		KASSERT(p == NULL, ("Too many references to event"));
ep->func = func;		ep->func = func;
ep->arg = arg;		ep->arg = arg;
mtx_lock(&g_eventlock);		mtx_lock(&g_eventlock);
TSHOLD("GEOM events");		TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events);		TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
wakeup(&g_wait_event);		wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags \|= TDP_GEOM;		curthread->td_pflags \|= TDP_GEOM;
thread_lock(curthread);		thread_lock(curthread);
curthread->td_flags \|= TDF_ASTPENDING;		curthread->td_flags \|= TDF_ASTPENDING;
thread_unlock(curthread);		thread_unlock(curthread);
		}

		void
		g_post_event_ep(g_event_t func, void arg, struct g_event *ep, ...)
		jhbUnsubmitted Not Done Inline Actions Why did you expose `wuflag` in this API? I would perhaps instead just have a new `g_waitfor_event_storage` if someone needs the `EV_WAKEUP` variant so that it mirrors the `g_post_event`/`g_waitfor_event` API. The 'waitfor' variant could be added in the future when it is needed. Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use `M_WAITOK` to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses `M_WAITOK`. jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
		impAuthorUnsubmitted Done Inline Actions I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense. imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
		{
		va_list ap;

		va_start(ap, ep);
		g_post_event_ep_va(func, arg, 0, ep, ap);
		va_end(ap);
		}


		static int
		g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)
		{
		struct g_event *ep;

		g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
		func, arg, flag, wuflag);
		KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
		("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
		ep = g_alloc_event(flag);
		if (ep == NULL)
		return (ENOMEM);
		if (epp != NULL)
		*epp = ep;
		g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0);		return (0);
}		}

int		int
g_post_event(g_event_t func, void arg, int flag, ...)		g_post_event(g_event_t func, void arg, int flag, ...)
{		{
va_list ap;		va_list ap;
int i;		int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines

View Options

sys/dev/sound/pcm/feeder_format.c

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines	for (n = 0; n < G_N_EVENTREFS; n++) {
break;		break;
}		}
}		}
if (TAILQ_EMPTY(&g_events))		if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events);		wakeup(&g_pending_events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
}		}

static int		struct g_event *
g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)		g_alloc_event(int flag)
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		impAuthorUnsubmitted Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). That's a good idea. imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of `g_vfs_done`: if (destroy) g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL); That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?) Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted. jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{		{
struct g_event *ep;		KASSERT(flag == M_WAITOK \|\| flag == M_NOWAIT,
		("Wrong flag to g_alloc_event"));

		return (g_malloc(sizeof(struct g_event), flag \| M_ZERO));
		}

		static void
		g_post_event_ep_va(g_event_t func, void arg, int wuflag,
		struct g_event *ep, va_list ap)
		{
void *p;		void *p;
u_int n;		u_int n;

g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag \| M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag;		ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) {		for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *);		p = va_arg(ap, void *);
if (p == NULL)		if (p == NULL)
break;		break;
g_trace(G_T_TOPOLOGY, " ref %p", p);		g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p;		ep->ref[n] = p;
}		}
KASSERT(p == NULL, ("Too many references to event"));		KASSERT(p == NULL, ("Too many references to event"));
ep->func = func;		ep->func = func;
ep->arg = arg;		ep->arg = arg;
mtx_lock(&g_eventlock);		mtx_lock(&g_eventlock);
TSHOLD("GEOM events");		TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events);		TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
wakeup(&g_wait_event);		wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags \|= TDP_GEOM;		curthread->td_pflags \|= TDP_GEOM;
thread_lock(curthread);		thread_lock(curthread);
curthread->td_flags \|= TDF_ASTPENDING;		curthread->td_flags \|= TDF_ASTPENDING;
thread_unlock(curthread);		thread_unlock(curthread);
		}

		void
		g_post_event_ep(g_event_t func, void arg, struct g_event *ep, ...)
		jhbUnsubmitted Not Done Inline Actions Why did you expose `wuflag` in this API? I would perhaps instead just have a new `g_waitfor_event_storage` if someone needs the `EV_WAKEUP` variant so that it mirrors the `g_post_event`/`g_waitfor_event` API. The 'waitfor' variant could be added in the future when it is needed. Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use `M_WAITOK` to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses `M_WAITOK`. jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
		impAuthorUnsubmitted Done Inline Actions I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense. imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
		{
		va_list ap;

		va_start(ap, ep);
		g_post_event_ep_va(func, arg, 0, ep, ap);
		va_end(ap);
		}


		static int
		g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)
		{
		struct g_event *ep;

		g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
		func, arg, flag, wuflag);
		KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
		("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
		ep = g_alloc_event(flag);
		if (ep == NULL)
		return (ENOMEM);
		if (epp != NULL)
		*epp = ep;
		g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0);		return (0);
}		}

int		int
g_post_event(g_event_t func, void arg, int flag, ...)		g_post_event(g_event_t func, void arg, int flag, ...)
{		{
va_list ap;		va_list ap;
int i;		int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines

View Options

sys/dev/sound/pcm/feeder_matrix.c

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines	for (n = 0; n < G_N_EVENTREFS; n++) {
break;		break;
}		}
}		}
if (TAILQ_EMPTY(&g_events))		if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events);		wakeup(&g_pending_events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
}		}

static int		struct g_event *
g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)		g_alloc_event(int flag)
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		impAuthorUnsubmitted Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage). That's a good idea. imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
		jhbUnsubmitted Not Done Inline Actions Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it always, but my other case in `g_vfs_done()` is optional and I will need to be able to free it in `g_vfs_close()` when a g_vfs object is freed without having its underlying storage destroyed. geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that. I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there... g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of `g_vfs_done`: if (destroy) g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL); That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?) Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted. jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{		{
struct g_event *ep;		KASSERT(flag == M_WAITOK \|\| flag == M_NOWAIT,
		("Wrong flag to g_alloc_event"));

		return (g_malloc(sizeof(struct g_event), flag \| M_ZERO));
		}

		static void
		g_post_event_ep_va(g_event_t func, void arg, int wuflag,
		struct g_event *ep, va_list ap)
		{
void *p;		void *p;
u_int n;		u_int n;

g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag \| M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag;		ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) {		for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *);		p = va_arg(ap, void *);
if (p == NULL)		if (p == NULL)
break;		break;
g_trace(G_T_TOPOLOGY, " ref %p", p);		g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p;		ep->ref[n] = p;
}		}
KASSERT(p == NULL, ("Too many references to event"));		KASSERT(p == NULL, ("Too many references to event"));
ep->func = func;		ep->func = func;
ep->arg = arg;		ep->arg = arg;
mtx_lock(&g_eventlock);		mtx_lock(&g_eventlock);
TSHOLD("GEOM events");		TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events);		TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock);		mtx_unlock(&g_eventlock);
wakeup(&g_wait_event);		wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags \|= TDP_GEOM;		curthread->td_pflags \|= TDP_GEOM;
thread_lock(curthread);		thread_lock(curthread);
curthread->td_flags \|= TDF_ASTPENDING;		curthread->td_flags \|= TDF_ASTPENDING;
thread_unlock(curthread);		thread_unlock(curthread);
		}

		void
		g_post_event_ep(g_event_t func, void arg, struct g_event *ep, ...)
		jhbUnsubmitted Not Done Inline Actions Why did you expose `wuflag` in this API? I would perhaps instead just have a new `g_waitfor_event_storage` if someone needs the `EV_WAKEUP` variant so that it mirrors the `g_post_event`/`g_waitfor_event` API. The 'waitfor' variant could be added in the future when it is needed. Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use `M_WAITOK` to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses `M_WAITOK`. jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
		impAuthorUnsubmitted Done Inline Actions I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense. imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
		{
		va_list ap;

		va_start(ap, ep);
		g_post_event_ep_va(func, arg, 0, ep, ap);
		va_end(ap);
		}


		static int
		g_post_event_x(g_event_t func, void arg, int flag, int wuflag, struct g_event **epp, va_list ap)
		{
		struct g_event *ep;

		g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
		func, arg, flag, wuflag);
		KASSERT(wuflag == 0 \|\| wuflag == EV_WAKEUP,
		("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
		ep = g_alloc_event(flag);
		if (ep == NULL)
		return (ENOMEM);
		if (epp != NULL)
		*epp = ep;
		g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0);		return (0);
}		}

int		int
g_post_event(g_event_t func, void arg, int flag, ...)		g_post_event(g_event_t func, void arg, int flag, ...)
{		{
va_list ap;		va_list ap;
int i;		int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines