RLIMIT_NOFILE >= 4096 when on TCL9 support #391
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dlmiles
force-pushed
the
master-upstream-20250219-rlimit
branch
from
6977807 to
9b6ac23
Compare
Collaborator
Author
|
minor change to fix text in printf and make tclmagic.c conform to file lint style This has been run through and tested on various NOFILE settings with TCL8 and TCL9 to ensure the intention is working. The TxGetInputEvent() still could do with a a view from a debugger. |
dlmiles
force-pushed
the
master-upstream-20250219-rlimit
branch
from
c386337 to
c0c814e
Compare
Collaborator
Author
|
rebased onto current master, pushed 1 more commit |
The old code would only work is the fileno(stream) returned an fd in the range 0 <= 19. It would silently fail, if the fd was in the range 20..1023 because FD_SET() would work and syscall select() would be limited to only look at the first 20 fd's. Ignoring any fd's higher even if set. This would theoretically cause high CPU usage due to select() never blocking because there are no active fd's in the fd_set as far as the kernel interprets the request and the kernel would immediately return. But reading the code the 1st argument to select() seems self limiting for no good reason. It should be fileno(stream)+1 as documented in man select(2). Added the assertion as well, because we are trying to allow magic to use fd's beyond the standard environmental limits. So it becomes an assertion condition if the fd is outside the range 0..1023 because the FD_SET() macro will not operate correctly / undefined-behaviour. I can't find any user of this func in the codebase right now. If you look at sim/SimRsim.c and the use of select() there, it is correctly using select() to wait on a single fd over there. This commit changes this code to match this correct usage.
This encapsulates the expectation the 'fd' is in the permitted range for the standard sizes 'fd_set'. This is so there is some form of detection with issues in this area, if the RLIMIT_NOFILE limit is increased.
There are numerous concerns with the original code from a read through. The #define TX_MAX_OPEN_FILES 20 is badly named, the txCommand.c uses fd_set to hold active FD for each device, and makes no attempt to bounds check the 'fd' or 'fd_set' of any TxAdd1InputDevice() is in range. The real name of this variable should be TX_MAX_INPUT_DEVICES as it related to the fixed compile time slots available for devices, each input device must have at least one active FD and it can be in the range that fd_set allows, which is 0..1023 on linux. So based on this being the original intention, due to the way the code is constructed and API calls made available, the file has been reworked to allow all these considerations at the same time. Now the design should be FD clean for FDs in the range 0..1023 instead of being artificially clamped to the first 20 FDs being valid input devices. Renaming of variable 'i' to 'fd' when it relates to an fd number, so all uses of FD_XXXX() should use fd, this helps clarify the different domain the number relates to. When 'i' is used it relates to the index into the txInputDevRec array. A large part of the concerns relate to trying to mix the two numbering domains interchangeably, just using a different name really clears up understanding to the reader. Other items that look like errors TxDelete1InputDevice() will shuffle-remove entries with no active FD, but it is iterating an array it is also modifying, so it looks like it would have incorrectly skipped the next item that should be inspected just after a shuffle-removal occurred. The various iterators that works from 0..TX_MAX_OPEN_FILES incorrectly limited the visibility of the routines to the first 20 FDs instead of the full FD_SETSIZE the TxAddInputDevice API call allows to be registered. Passing in TxAdd1InputDevice with an fd above 19 would have resulted in everything looking successful until select() was used, then the kernel would likely not sleep/wait because the input fd_set would look blank due to being clipped by nfds=TX_MAX_OPEN_FILES (instead of that plus 1). The use of TX_MAX_OPEN_FILES in select instead of TX_MAX_OPEN_FILES+1 for the 'nfds' field would have meant a device with fd=19 would not work as the design intended. The define definition has been removed from the module public header, I assume it was there for use by another module, but the incorrect select() usage has been corrected over there in a previous commit. So now the define can be maintained near the array it relates to. While the code might looks less 'efficient' as it is now sweeping all 1024 FDs when input devices during add/remove (hopefully there maybe some compiler auto-vectorization/tzcnt use there). The main event loop is slightly more 'efficient' (especially for the single device with input fd=0 case) as it will only check the 1 FD each input event loop iteration, not check all 20 FDs for data readyness everytime.
Original version would iterate exhaustively, even when it was not necessary. This version seeks to do the minimum amount of iteration work based on the information available.
dlmiles
force-pushed
the
master-upstream-20250219-rlimit
branch
from
c0c814e to
a742413
Compare
Collaborator
Author
|
a742413 adjustment to ifdef placement to account for platforms without rlimit so the type |
urish
added a commit
to TinyTapeout/ttihp25a-magic-fill
that referenced
this pull request
Mar 21, 2025
Increase ulimit and include the following patch: RTimothyEdwards/magic#391 > bit out of range 0 - FD_SETSIZE on fd_set
RTimothyEdwards
approved these changes
Apr 9, 2025
Owner
RTimothyEdwards
left a comment
RTimothyEdwards
left a comment
There was a problem hiding this comment.
Looks good to me, as far as I understand what's being done.
Owner
|
Pulled and merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Including defensive counter measures for areas of magic that don't support FDs >= 1024.
Any suggestions on getting to see TxGetInputEvent() in action ? Does not appear to be in use ?
Comments around this item #374 (comment)