Skip to content

Conversation

@IceTDrinker
Copy link
Member

  • we run in a cross process race condition which fucks up the key file
  • no rust crate seems to help and linux locks are just a fucking mess
  • also avoid truncating file when we are going to write to it, get a lock first

Added the key gen for the test as well to avoid issues

- we run in a cross process race condition which fucks up the key file
- no rust crate seems to help and linux locks are just a fucking mess
- also avoid truncating file when we are going to write to it, get a lock
first
@IceTDrinker IceTDrinker requested a review from nsarlin-zama March 7, 2025 10:32
@cla-bot cla-bot bot added the cla-signed label Mar 7, 2025
@IceTDrinker IceTDrinker marked this pull request as ready for review March 7, 2025 10:32
Copy link
Contributor

@nsarlin-zama nsarlin-zama left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


tfhe/src/keycache/mod.rs line 114 at r1 (raw file):

                let file = File::open(&path_buf).unwrap();
                // TODO Manage file locking for inter process stuff, unfortunately linux locks are a
                // mess and nothing seems to work

looks like there is a File::lock method in std that will be stabilized with 1.87: rust-lang/rust#136794
Maybe it will be worth retrying when this is released


tfhe/examples/utilities/generates_test_keys.rs line 59 at r1 (raw file):

    // Always generate those as they may be used in the different cases
    generate_ksk_keys(&KSK_PARAMS);

This is not directly linked to this pr but I think it should be "generate_ks_keys"

@IceTDrinker
Copy link
Member Author

@nsarlin-zama need changes or approved ?

Copy link
Contributor

@nsarlin-zama nsarlin-zama left a comment

Choose a reason for hiding this comment

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

Depends on if you want to fix the function name.

But this is not really important so approved anyways, thanks!

@IceTDrinker
Copy link
Member Author

Depends on if you want to fix the function name.

But this is not really important so approved anyways, thanks!

In the current case, even if the failure is rare I'd rather have it merged sooner than later :)

@nsarlin-zama
Copy link
Contributor

Oh I was thinking maybe instead of doing the truncate after lock thing we could use create_new and skip in case of error ?

@nsarlin-zama
Copy link
Contributor

I think it's specifically made for this kind of situations

@IceTDrinker
Copy link
Member Author

Oh I was thinking maybe instead of doing the truncate after lock thing we could use create_new and skip in case of error ?

Can be done later if you want to, I don't think it mattered much anyways as we pre generated the keys, not sure in terms of cache consistency what our various tests expect

@IceTDrinker
Copy link
Member Author

I mean create_new probably would have avoided the issue we had most likely, but then what are we supposed to do if the write fails ? Meaning two test processes have generated keys

@nsarlin-zama
Copy link
Contributor

nsarlin-zama commented Mar 7, 2025

I think if the second process just uses its own key but does not store it this is equivalent as what's happening here where the first key is overwritten by the second ?

Or worst case the second process could drop its generated key and reload the one from the file

@IceTDrinker
Copy link
Member Author

I think if the second process just uses its own key but does not store it this is equivalent as what's happening here where the first key is overwritten by the second ?

Or worst case the second process could drop its generated key and reload the one from the file

I don't think the second option would work with how the keycache is organized

@IceTDrinker IceTDrinker merged commit b0d7bb9 into main Mar 7, 2025
157 checks passed
@IceTDrinker IceTDrinker deleted the am/fix/shortint-keyswitching branch March 7, 2025 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants