Does lock require &mut self?

[timvisee]

Is there a good reason for lock() to require &mut self?

To compare, advice() is just &self.

[adamreichold]

I think this is a bug as there should be no external synchronization requirements, c.f. e.g. here. It also delegates to an internal method taking &self.

Would you mind creating a PR?

[timvisee]

Will do.

[RazrFalcon]

Interesting. @adamreichold shouldn't we make those methods &mut instead? We're mutating the memory map handle content after all, sort of.

[timvisee]

But can't the OS already flush pages at any point in time asynchronously?

[adamreichold]

What we are mutating are the page tables maintained by the operating system. Those are internally synchronized by the kernel (e.g. Linux's infamous mmap_sem) and hence POSIX mandates that mlock is safe to call from multiple threads.

This is similar to why for example File::set_len takes &self even though it definitely modifies the underlying file. The shared mutable state, i.e. the file system, is synchronized internally via the operating system. And this is strictly necessary as this particular state is shared with other processes and the kernel cannot allow one process to introduce data races into another.

This synchronization is also the reason why heavily multi-threaded programs on Linux suffer when a lot of mmap operations are involved, because those are always serialised by mmap_sem.

[adamreichold]

(There is even talk about sharing not only pages but also page tables between processes for reasons of efficiency for heavily containerized systems so that mlock in one process would affect the mapping in all other processes sharing it, c.f. https://lwn.net/Articles/895217/)

[RazrFalcon]

This is similar to why for example File::set_len takes &self

Oh wow. I didn't knew that. I guess as long as we're modifying something that isn't in full Rust control - there is no need for &mut self. Interesting edge case.

[timvisee]

Thanks for the quick response & merge. 😄

[timvisee]

@RazrFalcon Would you mind to push this to crates.io in the upcoming days?

[RazrFalcon]

Done.