Bug 276607

One question first: Do you know about mac_priority(4)? Is there any reason not to give the designated Jack user realtime privileges?

I'm thinking about removing the whole rtprio hack from the RC service file. Which should be possible now that all supported versions of FreeBSD have the mac_priority module. Going realtime would then be:

1. Add designated Jack user to the realtime group.
2. Change "jackd_args" in rc.conf to use realtime mode ("-R ...").

Am I missing something?


== Context ==

The default way to start Jack is now through the DBUS interface (jack_control), on demand. While I do not use the RC service anymore, I appreciate its usefulness and I'm willing to support it, as long as it works - against the recommendation of upstream.
But both the Jack server and the whole Jack ecosystem are now built on the assumption that threads can gain (and drop) realtime priority independently and by themself. Consequences are that

 - Not all threads of the Jack server and its clients _should_ have realtime priority
 - Threads will drop realtime priority and try to regain it later when needed

I'm sure that having only Jack server started with realtime priority can have positive effects in certain scenarios (like yours). But without the clients getting realtime priority too, its usefulness is limited. And it leads to problems similar to bug #263373. That's why I'd like to get rid of the rtprio hack completely, in favor of the more consistent user realtime privilege.


== About Buffer Balancing ==

The buffer balancing in Jack is an attempt to correct uneven progress between recording and playback channels. It does so by manipulating the playback channel, filling gaps or dropping samples. Please note that this can be both the cause or a symptom of the audible "knacks". And yes, sometimes it overreacts. The upcoming version of Jack will have a different approach.
Apart from realtime priority, there's some other possible workarounds:

 - Start Jack without a recording device, if you don't need it
 - Drop CPU intensive workloads to lower priority (e.g. run poudriere with idle priority)
 - Disable vchans and go bitperfect if Jack is the only consumer of the device
 - Go for smaller blocksize of the device (hw.snd.latency, hw.usb.uaudio.buffer_ms)
 - Make the Jack period a multiple of the blocksize (depends on your hardware)

(In reply to Florian Walpen from comment #1)

No, I missed the mac_priority. It looks like that's the perfect solution! I just tried it and it works - although the manpage says it's from Rel. 14.0, and I have 13.2.

With that method jackd brings only one thread into rtprio - and it is NOT the one as above.

I'm currently running it without rtprio, to see if the "buffer balancing" reappears - but that may simply have been caused by a week-old firefox filling up ressources or whatever else. After starting and stopping jackd a couple of times my desktop did finally crash, and I couldn't investigate further - maybe it was already corrupted in one way or another.
And while I do analyze server crashes, I don't care much about desktops failing occasionally - it's mostly not reproducible, and there is too much random activity happening behind the curtain, even without DBUS.
Anyway this is not a DAW, just a little add-on to finally replace my failing stereo&tapedeck.

So yes, I'd recommend remove the rtprio hack.

Created attachment 248109 [details]
Remove obsolete realtime hack for RC service.

Removed the rtprio(1) hack from RC service, added a warning in case `jack_rtprio` variable is defined, and changed the pkg-message accordingly.

Hi Yuri, do you still start JACK as an RC service?
Now that all supported versions of FreeBSD have mac_priority(4), I think it's time to remove the rtprio(1) hack from the service file. Are you ok with that?

Could you have a look at my patch and maybe commit it if there are no issues? Apply with `git am`. Also I think we can close your bug #263373 with this. Thank you!

Comment 5 Yuri Victorovich 2024-02-01 05:37:23 UTC

Hi Florian,

We need to first wait for the maintainer of this port to respond.

Thanks,
Yuri

I am the maintainer - forgot to set the feedback flag, sorry.

Comment 7 Yuri Victorovich 2024-02-01 11:23:16 UTC

Florian,

Shouldn't the jack port define the user 'jack'?
Otherwise the users should be deciding what user to run it as, which isn't necessary for them to decide.

(In reply to Yuri Victorovich from comment #7)
> Shouldn't the jack port define the user 'jack'?
> Otherwise the users should be deciding what user to run it as, which isn't
> necessary for them to decide.

Yuri, AFAIK jack server has to run under the same user as the clients. It's not a system wide server for all users. Which means that if you run jack server as user `jack`, you'd have to run your audio applications also as user `jack`. Even `jackdbus` is started as the user currently logged into the DBUS "seat".

Does that answer your question?

Comment 9 Yuri Victorovich 2024-02-01 18:25:09 UTC

(In reply to Florian Walpen from comment #8)

Hi Florian,

This answers the question, thank you.

But I have another question:
Isn't using mac_priority(4) with Jack degrade the current functionality in a sense that now the main user has to be a member of the realtime group? Ideally realtime priority should be given only to specific processes that need it, and that's what is currently the case with Jack. But now all processes run by the user would be realtime, which isn't exactly desirable.

(In reply to Yuri Victorovich from comment #9)

Hi Yuri, yes an no :-)

> But now all processes run by the user would be realtime...

Not true. Any process and thread run by the user can _request_ to _gain_ realtime priority. Which will be granted, if the user has realtime privileges. But most applications won't. Actually I only know of Jack server and clients which do this, probably some other audio applications as well.

In principle it's a safety regression, that's true. Because now all of the user's processes _could_ gain realtime priority and DoS the system.

But if you read the whole thread here, the current solution doesn't work at all. Only one thread of the Jack server gets realtime priority, and it's not the one that needs it. The problems with clients not getting realtime priority you know from bug #263373.

So in the end there's currently only mac_priority to have working realtime priority with Jack and clients. If DoS safety is a concern, there is no option.

Allowing realtime for the user makes sense, because now some other audio tools also go realtime: audacious, ardour. They use rtprio with the FIFO option (visible by the attached F in 'top' - which is only reachable by the posix library - we don't have a native option for that).

Traditionally rtprio was risky, because if by malfunction a process gets into an endless loop, the cpu is effectively dead. But nowadays we mostly have multi-processors, so multiple threads would need to run an endless loop. 

Also, if DoS might be a concern, there are possible remedies: one can limit the number of CPUs to use for the respective user via cpuset(1), or one could limit the total amount of compute available to a user via rctl(8).

Comment 12 Yuri Victorovich 2024-02-01 22:40:22 UTC

Created attachment 248121 [details]
Updated patch

I would like to propose an updated patch.

Differences:
1. keep the jackd_rtprio allowing the user an explicit control over rtprio being enabled in JACK
2. change the default to jackd_rtprio=YES
3. test whether all pre-requisite conditions are satisfied before starting JACK in the real-time mode. Explicit explanations leading users to how to correct problems are printed
4. disallow the user to use the -R argument and only add it in the script

I would also add the UPDATING entry explaining changes in jackd_rtprio.

(In reply to Yuri Victorovich from comment #12)

I like the idea of the updated patch, sounds quite user-proof. But don't we end up having both realtime (-R) and no realtime (-r) flags in the jackd args? Don't know what Jack does in that case...

I won't have time to test it until this weekend.

Comment 14 Yuri Victorovich 2024-02-01 23:52:16 UTC

Created attachment 248123 [details]
Updated patch v2

Hi Florian,

You are right, now I've added the check for -r as well.

I tested all bits that were added or changed, but you are welcome to test.


Thanks,
Yuri

Yeah, the explanations are wonderful, thats taking people by the hand...

But we need to check for whitespace before the -R, because arbitrary names could appear in the commandline.

Also, it seems to me this leads now to something like "-R -r ..." which will probably not work as intended.

Thinking... our general stance has always been to not hinder people from shooting themselves in the foot if they insist to. 
So I would do away with the $jackd_rtprio, not tamper with the commandline, just parse it for \s-R\s (there must be whitespace behind because the oss selection must follow) and then do all these nice checks and warnings, which are really helpful.

Just noticed: there is also a second -R option possible for the "NETONE" backend - I have no idea what that is or if it might be used.

Good. :)
Check, the -qw option.
Check, the -r handled.

Now just remove the -r from the default commandline at the top.

Comment 17 Yuri Victorovich 2024-02-02 08:45:50 UTC

Created attachment 248131 [details]
Updated patch v3

Indeed, -r was there by mistake.

(In reply to Yuri Victorovich from comment #17)

Please note that the user-provided jackd args could also have `--realtime` instead of `-R` and `--no-realtime` instead of `-r`. The `--rate` parameter often comes in the form of `-r 48000`. And I can't check at the moment, but if we're really unfortunate, jackd accepts combined short form args like `-rv` for `-r -v`...

Comment 19 Yuri Victorovich 2024-02-02 19:43:27 UTC

(In reply to Florian Walpen from comment #18)

Yes, 'grep -qw' takes care of this. Please test.


Yuri

(In reply to Yuri Victorovich from comment #19)

Yuri, I don't think you understood the implications of my previous comment. Ok, let's test...


These are valid jackd parameters (minus realtime flag), but it fails now with a misleading error message:

jackd_args="-d oss -p 768 -r 48000"

> JACK failed to start: jackd_args contains -r which isn't allowed: it is set automatically only when needed

If someone used the long form of the realtime flag until now, we start jackd with conflicting realtime flags, no error message:

jackd_args="--no-realtime -doss -p768 -r48000"

> Starting the daemon, user=flo args="-R --no-realtime -doss -p768 -r48000"

Same goes for concatenated global jackd flags:

jackd_args="-rv -doss -p768 -r48000"

> Starting the daemon, user=flo args="-R -rv -doss -p768 -r48000"

Currently jackd accepts conflicting realtime flags and ignores all but the last flag. But I'm not comfortable to rely on this undocumented behavior, since that may change in future versions of jackd.

Mind you, these are not esoteric ways of setting the realtime flags and sample rate, you will find examples in the man page of jackd and on the internet.


Also I don't think we should make realtime priority the default. Users may feel obliged to enable it, even when it's not needed for their use case.


As an alternative, how about the following proposal:

 - Get rid of the `jackd_rtprio` variable.
 - Don't mess with the `jackd_args` given by the user.
 - Detect the intention to use realtime priority, by either
   -> `jackd_rtprio` is set to `YES`, OR
   -> `jackd_args` contains `oss` AND
     -> `jackd_args` contains `-*R` OR
     -> `jackd_args` contains `--realtime`
 - When detected, do the checks regarding mac_priority and user privileges

IMHO it's ok for the detection to be conservative. Better let someone run without checks and warnings in non-obvious cases. But never mess with peoples valid `jackd_args` because of false positives.

Comment 21 Yuri Victorovich 2024-02-04 19:47:48 UTC

Created attachment 248189 [details]
Updated patch v4

Hi Florian,

Indeed, the script had this problem.
It is now resolved.

Thanks,
Yuri

(In reply to Yuri Victorovich from comment #21)

Hi Yuri, this only handles one of my concerns above. I'll come up with an updated patch some time next week. We also have to make the realtime privilege test more robust.

Comment 23 Yuri Victorovich 2024-02-05 00:27:06 UTC

> Also I don't think we should make realtime priority the default.

We can make it NO by default.

I just don't like situations when you are trying to start something and it crashes, or prints strange messages, and I have to spend hours reading the docs for something that is very clearly invalid and is a common pitfall. All users need to read docs in depth and re-discover same common things, instead of just one software distributor providing software that just runs without the need for multiple discovery.

Comment 24 Yuri Victorovich 2024-02-05 01:00:18 UTC

Created attachment 248192 [details]
Updated patch v5

Florian,

I think that the attached patch addresses all your concerns.

It introduces the jackd_expert variable.
When jackd_expert=YES - config is accepted by default.
When jackd_expert=NO - config is checked, -R and -r are added when needed as was done above.
jackd_expert=NO is the default because 99% of users aren't experts and need JACK to just run.

jackd_rtprio=NO is made to be the default.

Please let me know what you think.

Thanks,
Yuri

Created attachment 248220 [details]
Remove obsolete realtime hack for RC service, with checks.

Yuri, don't get me wrong, I like your ideas and efforts. And I'm with you about guiding users and useful defaults.

But your checks of `jackd_args` don't really work, as stated above. And the changed semantics of `jackd_args` mean everybody is interrupted and has to adjust `/etc/rc.conf`, even if they don't need realtime priority.

That's why I'd root for the much simpler approach in this updated patch:
 - Keep semantics of `jackd_args`.
 - Detect when users intend to run with realtime priority.
   - In that case, do some checks and guide them through migration.
 - Only use `jackd_rtprio` for detection of intent, deprecate.

Even for detection of the realtime flags I had to wrestle a bit, hope it's tight now. We don't want false positives there. Also I removed the `jackd_user` default value of `root`, which sabotaged your user sanity check. The examples of the realtime flags should be more obvious now, `--no-realtime`.

What do you think?

Comment 26 Yuri Victorovich 2024-02-11 17:04:44 UTC

Committed, thanks!

Comment 27 commit-hook 2024-02-11 17:05:02 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=100a9852dd8957a1d7a337858a3319da693915c9

commit 100a9852dd8957a1d7a337858a3319da693915c9
Author:     Florian Walpen <dev@submerge.ch>
AuthorDate: 2024-02-11 17:03:42 +0000
Commit:     Yuri Victorovich <yuri@FreeBSD.org>
CommitDate: 2024-02-11 17:03:42 +0000

    audio/jack: Remove obsolete realtime hack for RC service, with checks

    PR:             276607

 audio/jack/Makefile       |  2 +-
 audio/jack/files/jackd.in | 43 ++++++++++++++++++++++++++++++++++++-------
 audio/jack/pkg-message    | 11 +++++++----
 3 files changed, 44 insertions(+), 12 deletions(-)

Thanks for the commit, Yuri. But why did you re-add the following lines?

> : ${jackd_user="root"}
> : ${jackd_rtprio="NO"}

The `jackd_rtprio` variable is just useless now, it doesn't need a default value. We only check it in the code to migrate existing users to mac_priority realtime privileges.

The default value for `jackd_user` suggests to run jackd as root, which is a bad idea, and it sabotages your check that the user has set a valid `jackd_user` in /etc/rc.conf.

It's not a big problem, I'll remove those lines with the next audio/jack update. But I don't understand it, care to explain?


Peter, could you make an additional comment in your forum thread?
Might be a good idea to clarify how this was resolved. Thank you!

Comment 29 Yuri Victorovich 2024-02-12 16:32:47 UTC

(In reply to Florian Walpen from comment #28)

Hi Florian,

> The `jackd_rtprio` variable is just useless now [...]

Both jackd_rtprio and jackd_rtprio appear in the jackd.in

It's customary to set some defaults for variables, otherwise the default is an undefined value.

The empty default can be set as well. But to use a variable and not to declare it isn't a good practice IMO.

Yuri

(In reply to Yuri Victorovich from comment #29)

Ok, I disagree because shell script doesn't care about undefined variables, but I understand the sentiment. Thanks for the explanation.

So on next update I'll set default of `jackd_user` to be empty, and remove `jackd_rtprio` completely from jackd.in once your commit has hit the quarterly branch. By then we should have reached all current users for migration.

Comment 31 Yuri Victorovich 2024-02-12 17:55:01 UTC

(In reply to Florian Walpen from comment #30)

> Ok, I disagree because shell script doesn't care about undefined variables [...]

But this isn't just a shell script.
This is a RC script. It can officially have arguments set by users in a special way, through /etc/rc.conf. It should be clear from looking at the top of the RC script what the arguments are, and what their default values and definitions are.

Just using some variable somewhere in the script without declaring it at the top can and will cause confusion. Not everybody has time and ability to read through scripts.

If you would look at other RC scripts - virtually all of them define all variables in 2 places: as a list with human readable explanations, and as a list with default values.

There is no strict policy for this, but this is a convention which makes a lot of sense.

(In reply to Yuri Victorovich from comment #31)

> Just using some variable somewhere in the script without declaring it at
> the top can and will cause confusion. Not everybody has time and ability
> to read through scripts.

Understood. And I will keep `jackd_user` there, albeit with a different default. But right now we have the opposite problem, `jackd_rtprio` is declared at the top, and not used. People will be confused because it has no effect, except for triggering the realtime checks.

On a side note, I think a man page section would be a better place to document RC services.

Anyway, no harm done, let's move on.

Cheers

Florian

Comment 33 Yuri Victorovich 2024-02-12 22:32:12 UTC

(In reply to Florian Walpen from comment #32)

jackd_rtprio does appear in files/jackd.in, so it is used.