Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait for ueventd to create loop device on Android #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@tiann
Copy link
Author

tiann commented Feb 13, 2023

I will try it with inotify :)

@tiann
Copy link
Author

tiann commented Feb 13, 2023

We need this PR : notify-rs/notify#470 to be merged first

@flxo
Copy link
Contributor

flxo commented Feb 13, 2023

The loopdev crate has a neat small list of dependencies. Not sure if adding inotify (for target_os = "android") is the way to go. Using the libc interface directly would also be possible and the use case here is simple enough.

Sidenote: I use inotify-rs on Android in production.

@0xpr03
Copy link

0xpr03 commented Feb 13, 2023

If you only need inotify [the linux event API], and no other OS support, then notify [the crate] is probably overkill and inotify-rs could be enough.

@tiann
Copy link
Author

tiann commented Feb 14, 2023

And i did a benchmark:

Without inotify:

cost: 20.716512ms
cost: 35.034µs
cost: 20.799764ms
cost: 20.795166ms
cost: 20.71757ms
cost: 20.412802ms
cost: 20.288493ms
cost: 20.745402ms
cost: 20.602376ms
cost: 20.689046ms

Average: 18ms

With inotify:

cost: 11.391805ms
cost: 15.421µs
cost: 6.936767ms
cost: 2.65031ms
cost: 12.248576ms
cost: 8.989787ms
cost: 7.116781ms
cost: 9.271932ms
cost: 15.299µs
cost: 1.726196ms

Average: 6ms

Both are release build for arm64-v8a, It is about 12ms faster.

@tiann
Copy link
Author

tiann commented Feb 14, 2023

Maybe we change the interval wait time to 1ms without inotify, and it should also work

Copy link
Contributor

@flxo flxo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure wether a fast poll with an initial may be a better idea.

There's a hardcoded 2s timeout that can bite.

Is WatchMask::LCREATE firing? Think there can be the case where the looped files are are already present, free and yielded by LOOP_CTL_GET_FREE.

@tiann
Copy link
Author

tiann commented Feb 20, 2023

What should we do now? I changed it to loop wait 1ms

@flxo
Copy link
Contributor

flxo commented Feb 20, 2023

hi @mdaffin. What's your opinion on that?

@tiann
Copy link
Author

tiann commented Apr 17, 2023

I've been using this fix for quite some time now: https://github.com/tiann/KernelSU/blob/main/userspace/ksud/Cargo.toml#L38, but I had to maintain my own branch. Would you consider merging it in? I'm open to any suggestions you may have.

@tiann
Copy link
Author

tiann commented Apr 18, 2023

@mdaffin Could you please check this?

@mdaffin
Copy link
Owner

mdaffin commented Apr 18, 2023

I am not a huge fan of spinning on a 1ms delay for up to 2 seconds. It is all well and good if your tests are coming back in ~6ms, but then that raises the question as to what situation it could take to to 2 seconds to update. If we want a delay loop I think it might be better for an exponential back off instead - ie double the delay until a reasonable limit or the timeout is hit? Or does that sound like too much complexity?

Sorry for the delay in looking at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants