Bug 276614 - graphics/mesa-dri:fix os_same_file_description warning
Summary: graphics/mesa-dri:fix os_same_file_description warning
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-x11 (Nobody)
URL: https://gitlab.freedesktop.org/mesa/m...
Keywords:
Depends on:
Blocks:
 
Reported: 2024-01-25 17:00 UTC by Ivan Rozhuk
Modified: 2024-03-30 04:34 UTC (History)
7 users (show)

See Also:
manu: maintainer-feedback-


Attachments
patch (3.46 KB, patch)
2024-01-25 17:00 UTC, Ivan Rozhuk
no flags Details | Diff
patch (2.84 KB, patch)
2024-02-17 21:20 UTC, Ivan Rozhuk
rozhuk.im: maintainer-approval?
Details | Diff
syslog() every time missing kcmp() can leak memory (5.98 KB, patch)
2024-03-26 13:56 UTC, Jan Beich
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ivan Rozhuk 2024-01-25 17:00:42 UTC
Created attachment 247954 [details]
patch

This adaptation of https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6881
Comment 1 Emmanuel Vadot freebsd_committer freebsd_triage 2024-01-25 18:40:27 UTC
NAK, use the kcmp(2) implementation or wait for it to be in the release that you use.
Comment 2 Ivan Rozhuk 2024-01-25 19:00:46 UTC
Nope, stop shooting in FreeBSD users on non CURRENT :)
You leave all users on 13, 14 without fix.

Until all supported FreeBSD versions does not have kcmp() this patch should be exist.
Comment 3 Emmanuel Vadot freebsd_committer freebsd_triage 2024-01-25 19:16:53 UTC
(In reply to Ivan Rozhuk from comment #2)

I'm shooting no one, this warning is harmless.
Comment 4 Ivan Rozhuk 2024-01-25 21:01:44 UTC
(In reply to Emmanuel Vadot from comment #3)

If it harmless - why kcmp() added to kernel?

I do not understand why you decline this port improvement.
Comment 5 Emmanuel Vadot freebsd_committer freebsd_triage 2024-01-26 06:04:30 UTC
(In reply to Ivan Rozhuk from comment #4)

Adding kcmp(2) is an improvement to the kernel and yes it will help some mesa driver. Not having it will not cause big trouble.
This patch will never be upstreamed now that we have kcmp and I don't want to keep non-upstreamed patches.
Stop re-opening this PR, it will never happen.
Comment 6 Jan Beich freebsd_committer freebsd_triage 2024-02-01 02:43:36 UTC
Meanwhile, the warning continues to scare users with "bad things may happen".

https://old.reddit.com/r/freebsd/comments/1aflw4t/freebsd_14_amd_gpu_os_same_file_description_error/
Comment 7 Ivan Rozhuk 2024-02-01 14:47:27 UTC
(In reply to Emmanuel Vadot from comment #5)

I will not stop until this get fixed!

If you do not want to serve project and users - step aside and do not waste your and mine time.
Comment 8 Gleb Popov freebsd_committer freebsd_triage 2024-02-01 15:41:47 UTC
(In reply to Ivan Rozhuk from comment #7)
What's the point in being stubborn? You'll only get yourself banned. No one is going to push your patches in and you don't have power to do it yourself.
Comment 9 Ivan Rozhuk 2024-02-01 17:00:54 UTC
(In reply to Gleb Popov from comment #8)

1. This is not mine patch, it is Jan~s with few modifications/style changes.

2. Since I do nothing bad, after ban me we will go to public to discuss: why some peoples here hates FreeBSD desktop users, does not fix broken things, and does not let other peoples share their patches to fix broken things.

3. I always can register 100500 new accounts or switch to less toxic and more user friendly community.

Are you ready?
Comment 10 Alexey Dokuchaev freebsd_committer freebsd_triage 2024-02-05 08:23:33 UTC
(In reply to Ivan Rozhuk from comment #9)
> [...] why some peoples here hates FreeBSD desktop users, does not fix
> broken things, and does not let other peoples share their patches to
> fix broken things.
Yes, unfortunately, we've seen this attitude being exhibited more than once. :-(

I'm with Ivan here, esp. given that he's one of our long-term desktop users and the author of patches that make FreeBSD desktop experience smoother, while support from our staff X11 maintainers is often lacking.  Turning away people like him would be a big loss to the project.  It is generally bad to dismiss contributors, let alone threatening them with a ban.

Now that being said, can we (all) please get a little more constructive here?  I can understand Emmanuel's unrest with having to maintain some ~100-line patch.  Maybe there's a less intrusive way to not scare users until kcmp(2) becomes omnipresent?
Comment 11 Gleb Popov freebsd_committer freebsd_triage 2024-02-05 08:45:28 UTC
(In reply to Alexey Dokuchaev from comment #10)
> he's one of our long-term desktop users and the author of patches that make FreeBSD desktop experience smoother, while support from our staff X11 maintainers is often lacking.  Turning away people like him would be a big loss to the project.

Let's do a merit check. What patches are you talking about?
Comment 12 Emmanuel Vadot freebsd_committer freebsd_triage 2024-02-05 08:46:23 UTC
(In reply to Alexey Dokuchaev from comment #10)

> Yes, unfortunately, we've seen this attitude being exhibited more than once. :-(
> I'm with Ivan here, esp. given that he's one of our long-term desktop users and 
> the author of patches that make FreeBSD desktop experience smoother, while 
> support from our staff X11 maintainers is often lacking.

 Being alone in the x11@ team for the past ~2 years, I think that I done a pretty correct job at maintaining the ports while still continuing to help people with drm-kmod when problems appears so this comment is hurting me a bit.
 Also I use FreeBSD as my main desktop for 20 year now so don't think I hate desktop users :)

> Turning away people like him would be a big loss to the project.
> It is generally bad to dismiss contributors, let alone threatening them with a ban.

 It is generally good to dismiss contributors with aggressive attitude though.

> Now that being said, can we (all) please get a little more constructive here?  
> I can understand Emmanuel's unrest with having to maintain some ~100-line patch.

 Maintaining a patch isn't the problem (even if I want all patches to be upstreamed ideally), the main problem is that this patch wasn't reviewed, wasn't accepted upstream (unlike the use of kcmp(2) which was merge yesterday) and I don't plan to review it. Having kcmp(2) is an improvement, not a bugfix (could be wrong, feel free to correct me).

> Maybe there's a less intrusive way to not scare users until kcmp(2) becomes omnipresent?
Comment 13 Emmanuel Vadot freebsd_committer freebsd_triage 2024-02-05 08:47:33 UTC
(In reply to Gleb Popov from comment #11)

No merit check needed, you can upstream a lot of patches, new utils and new drivers but that doesn't give you the right to behave this way. No one should ever behave this way.
Comment 14 Alexey Dokuchaev freebsd_committer freebsd_triage 2024-02-05 09:44:18 UTC
(In reply to Gleb Popov from comment #11)
> Let's do a merit check. What patches are you talking about?
Of his recent works, an alternative, more robust and performant FAM backend for `devel/glib20' comes to mind (see bug #255707 and earlier), among other PRs he had filed.  Sticking with the project for a long time, one learns to recognize people's names.  You can also check his insightful comments on OpenNET.
Comment 15 Gleb Popov freebsd_committer freebsd_triage 2024-02-05 09:59:43 UTC
(In reply to Alexey Dokuchaev from comment #14)
Just as I thought. This homegrown GFileMonitor backend isn't upstreamed and I'm 99% sure there are no users other than himself. I will remove this option once I find a time to convert libinotify-kqueue into a GFileMonitor backend. I also got a feeling that all Ivan's contibutions are ad-hoc, hacky and serve only him, not the others. But maybe I'm too subjective on that.

Anyways, Ivan's merits are miniscule compared to work done by Jan, Emmanuel and the rest of the x11 team. Add to that his aggresive tone and the fact that there is no critical problem with having no kcmp implementation.
Comment 16 Ivan Rozhuk 2024-02-05 16:28:34 UTC
(In reply to Gleb Popov from comment #15)

You are trying to marginalize me and what I do. It looks like this is your best argument in defending your position.

1. I'm here because it is broken or absent.
This means that upstream or/and maintainer screwed up.

2. I solve my problems exclusively at my own expense. (with rare exceptions when I do the paid work)
If you don't like it, you can hire me and then you can tell me what and how to do.

3. I am not interested in the status, achievements, merits and everything else with regard to the personality (mine or opponents).
I am here to solve a specific technical problem.

4. The whole system of ports is a set of crutches and hacks to allow software to be written mainly for linux will compile and work for freebsd.
By calling my work a hack - you do not need to separate this from all other ports.

5. Before saying that the results of my work I use only me - study the question, 99% that you use the code written by me.



The whole story with GFileMonitor has been and still remains an example of how much the gnome community does not care about the FreeBSD of users and how much the FreeBSD community does not care about its own users.
This is a huge failure, for which all involved should be ashamed, but for which no one was ever evened, and when they brought them a ready made decision in the form of a patch, then the answer was: “Give us this in the form of a series of single line changes” - this is just a spit in The face is personally to me, a bureaucratic trick. And here this patch hung years before it was turned on.
I understand that for the gnome - FreeBSD is alien and they don’t care, but I don’t understand why the FreeBSD community is so insolvent that the finished patch cannot just accept for years.


> Just as I thought. This homegrown GFileMonitor backend isn't upstreamed and I'm 99% sure there are no users other than himself. I will remove this option once I find a time to convert libinotify-kqueue into a GFileMonitor backend.

6 or 7 years have passed already, and you have just gathered to do something!?
Here he is an example of complete failure.

libinotify-kqueue is a good solution, only it is late for 8 years.
Make sure your solution is not only easier to maintain but it also works better before doing something.
Because it seems that it doesn’t matter for maintainers how it works, it is important that it is easy to maintain.
Comment 17 Ivan Rozhuk 2024-02-05 16:31:04 UTC
I keep this open, since none proof provided that it is cosmetic warning.
Comment 18 Gleb Popov freebsd_committer freebsd_triage 2024-02-05 16:37:44 UTC
(In reply to Ivan Rozhuk from comment #16)
> If you don't like it, you can hire me and then you can tell me what and how to do.

I can as well just ignore you from now. That'd be more cost-efficient.

> I am here to solve a specific technical problem.

The technical problem was solved in most appropriate way by implementing kcmp in the kernel. And this work was not done by you. Your change is a workaround, which you are now trying to push for some reason.

Anyways, I won't argue anymore.
Comment 19 Ivan Rozhuk 2024-02-05 16:44:37 UTC
(In reply to Gleb Popov from comment #18)

> The technical problem was solved in most appropriate way by implementing kcmp in the kernel.

It is only in CURRENT.
Do you understand that? Or you do not care users that is not you?


> And this work was not done by you. 

I do not want to do something, read p1 from prev message.


> Your change is a workaround, which you are now trying to push for some reason

This is not mine code, it is Jan~s.
Do you understand this or just want to try to insult me again?

And this is not workaround, this is kcmp() user space implementation.
Are you able to understand the difference?
Comment 20 Jan Beich freebsd_committer freebsd_triage 2024-02-05 23:36:43 UTC
(In reply to Emmanuel Vadot from comment #12)
> the main problem is that this patch wasn't reviewed

It was by you (earlier version) and Val (later version). Upstream is hestitant because:
- the patch is non-trivial (new code) but still simple
- they have no in-house FreeBSD experts, so can only rubberstamp
- other BSDs disable checking kernel pointers for non-root
- context fatigue from too many review comments

> not a bugfix

ports c14ad44ccafc indirectly points to https://gitlab.freedesktop.org/mesa/mesa/-/issues/2882 and https://gitlab.freedesktop.org/mesa/mesa/-/issues/2413

os_same_file_description() usage is limited to OpenGL drivers as Vulkan likely lets consumers do the same thing in an explicit way.
Comment 21 Ivan Rozhuk 2024-02-06 03:05:23 UTC
I can confirm that:
1. Patch work as expected and works for me 10+ days.
2. I check it in test app with different fd types and it detect fd~s obtained via dup().
3. No need to rebase patch after latest mesa update.
Comment 22 Ivan Rozhuk 2024-02-17 21:20:12 UTC
Created attachment 248550 [details]
patch
Comment 23 Emmanuel Vadot freebsd_committer freebsd_triage 2024-02-19 18:11:44 UTC
So since I know more about Watning than a person from Mesa (your words) can you show me a bug or graphics misbehavior that is happening on FreeBSD without kcmp (14.0 for example) and that this patch fixes ?
If you're not able to there is no way I'm commiting this.
Comment 24 Ivan Rozhuk 2024-02-19 20:16:28 UTC
(In reply to Emmanuel Vadot from comment #23)

Ask here: https://gitlab.freedesktop.org/mesa/mesa/-/commit/923758a5c223497c45de3ecf13b560423718157d
Comment 26 Emmanuel Vadot freebsd_committer freebsd_triage 2024-02-19 20:31:18 UTC
No.
If you have a proven bug on FreeBSD without kcmp(2) that this patch fixes it I'm willing to review it and commit it, until now I'm closing this bug as a patch to a port is supposed to fix a bug (especially a non-upstreamed patch).
Comment 27 Ivan Rozhuk 2024-02-19 20:36:16 UTC
Patch fixes: "os_same_file_description warning" message.
Why it is important you can read using provided links.
Comment 28 Emmanuel Vadot freebsd_committer freebsd_triage 2024-02-19 20:40:54 UTC
(In reply to Ivan Rozhuk from comment #27)

Are you saying that the patch is the same as if we removed the printfs ?
Does it fixes an actual bug that you are able to trigger running a port on FreeBSD ? No.
So stop waiting my time by being such an asshole and a pain in the ass to work with.
The only thing you gained from doing this is that I'll never look at one of your PR from now on even no matter how good or bad they are.
Comment 29 Ivan Rozhuk 2024-02-19 20:51:49 UTC
(In reply to Emmanuel Vadot from comment #28)

I want to say that this patch makes mesa behave the way it was originally intended by its authors.
If you don't understand something, you can ask them personally, I did the work and gave you links to read explanatory messages and contacts.

Your refusal is direct discrimination against all users of stable branches.

By promising to ignore my work in the future, you are directly stating that your personal motivation is contrary to the interests of this community.
Comment 30 Emmanuel Vadot freebsd_committer freebsd_triage 2024-02-19 20:59:18 UTC
(In reply to Ivan Rozhuk from comment #29)

> I want to say that this patch makes mesa behave the way it was originally intended by its authors.

You have no idea about this.

> If you don't understand something, you can ask them personally, I did the work and gave you links to read explanatory messages and contacts.

You don't seems to understand anything. 

> Your refusal is direct discrimination against all users of stable branches.

No, it's clearly a discrimination against you.

> By promising to ignore my work in the future, you are directly stating that your personal motivation is contrary to the interests of this community.

The community on this issue is only you, if any other user would have report a bug in past 2-3 years since this "issue" was present I would have gave this a better look, the fact that no bug report was ever opened is a proof that this patch is useless and that adding kcmp(2) is only an improvement and not a bug fix.

If your not able to understand this, you really have a problem.
Comment 31 Ivan Rozhuk 2024-02-19 21:17:24 UTC
(In reply to Emmanuel Vadot from comment #30)
> You have no idea about this.

Ask the authors who added the warnings, they will explain everything to you.


> The community on this issue is only you

What about https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=276614#c6 ?
Why other peoples CC to this?


> if any other user would have report a bug in past 2-3 years since this "issue" was present

Warning commits are just over a year old.


> No, it's clearly a discrimination against you.

Emmanuel, think again, it’s not just me here, at least the author is Jan Beich, and several more people, of whom only Gleb Popov is not expecting this change.
At least this is 3 against 2 of those who spoke here.

All the work has already been done, and you are capricious like a little child.
Comment 32 Joan Naylor 2024-03-20 04:00:28 UTC
MARKED AS SPAM
Comment 33 Maru Kari 2024-03-25 03:48:39 UTC
MARKED AS SPAM
Comment 34 Yusuf Khan 2024-03-25 04:26:19 UTC
(In reply to Maru Kari from comment #33)
> error

I know I shouldn't fan the flames but...there is no error, your system should function just fine(with a minor memory leak?) unless your using iris/crocus and you decide to use an insanely weird compositor that exports KMS buffers through GBM and needs to use that feature for some odd reason(pretty sure such a compositor doesnt exist). If there is any issue with your system, its not from this warning.
Comment 35 Alastair Hogge 2024-03-25 04:35:16 UTC
(In reply to Jan Beich from comment #6)

+1

After 20+ years, having finally getting a couple of normies in meat-space to adopt FreeBSD on their hardware, this warning creates unnecessary grey matter haze for users. Sympathies with users on -STABLE, and releases, always!

For the cause!
Comment 36 Ivan Rozhuk 2024-03-26 03:39:15 UTC
(In reply to Yusuf Khan from comment #34)

Merge fix for this warning cost NOTHING.
But it is very high price for some how for maintainer.
Comment 37 Yusuf Khan 2024-03-26 04:26:39 UTC
(In reply to Ivan Rozhuk from comment #36)
I am guessing you are trying to target non-CURRENT? I don't know the full FBSD criteria for something going into the stable releases but I think that they only want *bug* fixes instead of routine annoyance fixes like this is. I'd personally drop trying to convince any maintainer of committing stuff they dont want to commit and start doing things more productive things, neither you nor the maintainers seem to care about the memory leak part of it, and the maintainers dont care about the warning appearing.
Comment 38 Ivan Rozhuk 2024-03-26 04:37:10 UTC
(In reply to Yusuf Khan from comment #37)

CURRENT/STABLE/RELEASE - it is out of scope in ports context.
In terms of end users only release or possible stable make sense.
If project does not care about release and stable users - it will not have users outside it is own developers.

More productive is drop FreeBSD due to inadequate ports maintainers and switch to linux, where most of problems that I was report (some with patches) in this bugtracker is not exist.
Comment 39 Yusuf Khan 2024-03-26 05:13:25 UTC
(In reply to Ivan Rozhuk from comment #38)
So you want STABLE to be a CURRENT but not too CURRENT so that bugs dont sweep in? I think that is a completely different ideology to what STABLE is about in FreeBSD.

>switch to linux
The grass is always greener on the other side.
Comment 40 Ivan Rozhuk 2024-03-26 06:36:47 UTC
(In reply to Yusuf Khan from comment #39)

Ports is not for CURRENT users only, it is for all supported versions.
Comment 41 Jan Beich freebsd_committer freebsd_triage 2024-03-26 13:56:29 UTC
Created attachment 249495 [details]
syslog() every time missing kcmp() can leak memory

(In reply to Yusuf Khan from comment #34)
> (with a minor memory leak?)

The original leaks weren't "minor", see https://gitlab.freedesktop.org/mesa/mesa/-/issues/1426 and https://gitlab.freedesktop.org/mesa/mesa/-/issues/2270 . Would be nice to collect more data by unthrottling the warning (see attached patch) and checking what triggers it. However, I'm not sure how to catch leaks over long-lived graphical sessions.

For example, nesting sway/mesa-dri from 13.2 jail under sway-devel/mesa-devel from 15.0-CURRENT host suggests kcmp() is required at least 3 times (on the jail side).

# pkg install -qy sway
$ MESA_LOG=file sway
MESA: warning: iris: Kernel has no file descriptor comparison support: Invalid argument
MESA: warning: iris: Kernel has no file descriptor comparison support: No such file or directory
MESA: warning: iris: Kernel has no file descriptor comparison support: No such file or directory
[...]
Comment 42 Yusuf Khan 2024-03-26 14:16:19 UTC
(In reply to Jan Beich from comment #41)
Oh! I am in a bit of a rush right now but if those two issues are related to the winsys memory leak then I think that would be grounds for the maintainers to look at putting this into stable...still their prerogative though.

>MESA: warning: iris: Kernel has no file descriptor comparison support: Invalid argument

If sway isnt crashing with this warning then it should probably not be grounds to consider this patch, although if the memory leak is as severe as you say on certain applications, get more persuasive!
Comment 43 Ivan Rozhuk 2024-03-27 09:38:18 UTC
(In reply to Jan Beich from comment #41)
Sorry for off topic, but:
1. How you has detect mem leak?
2. How it is possible to leak in syslog()?
3. If it possible - is this libc (or where syslog() implemented) issue?
Comment 44 Jan Beich freebsd_committer freebsd_triage 2024-03-27 16:09:27 UTC
(In reply to Yusuf Khan from comment #42)
> although if the memory leak is as severe as you say on certain applications

I've challenged your severity claim but didn't state my own, only pointed at related upstream findings. The warning disappeared from my dogfood plate 3 years ago in ports c14ad44ccafc, so I didn't have time to gather much data. The ongoing cost of uncertainty in my own QA and when reporting various bugs upstream was too much to not just fix it.

(In reply to Ivan Rozhuk from comment #43)
> 1. How you has detect mem leak?

Indirectly by logging every (not just the first) failed os_same_file_description() call (with PID and errno) into /var/log/messages. This doesn't track the leaked memory size.

> 2. How it is possible to leak in syslog()?

Look inside the patch. It merely hooks ad hoc warnings into MESA_LOG facility and changes MESA_LOG from "file" to "syslog" by default. Logging into to stderr isn't reliable if Xorg is started by a greeter (graphical login manager aka display manager) and changing MESA_LOG_FILE isn't reliable with setuid applications such as Xorg (until bug 273161). Besides, Mesa warnings are kinda serious unlike the rest of stderr output after startx, which often includes regular and noisy applications.