On 08.02.21 16:27, Jan Kara wrote:
Hi Alexander! On Fri 05-02-21 16:31:54, Alexander Lochmann wrote:have you had the chance to review our results?It fell through the cracks I guess. Thanks for pinging. Let me have a look.On 15.10.20 15:56, Alexander Lochmann wrote:Hi folks, when comparing our generated locking documentation with the current documentation located in include/linux/jbd2.h, I found some inconsistencies. (Our approach: https://dl.acm.org/doi/10.1145/3302424.3303948) According to the official documentation, the following members should be read using a lock: journal_t - j_flags: j_state_lock - j_barrier_count: j_state_lock - j_running_transaction: j_state_lock - j_commit_sequence: j_state_lock - j_commit_request: j_state_lock transactiont_t - t_nr_buffers: j_list_lock - t_buffers: j_list_lock - t_reserved_list: j_list_lock - t_shadow_list: j_list_lock jbd2_inode - i_transaction: j_list_lock - i_next_transaction: j_list_lock - i_flags: j_list_lock - i_dirty_start: j_list_lock - i_dirty_start: j_list_lock However, our results say that no locks are needed at all for *reading* those members. From what I know, it is common wisdom that word-sized data types can be read without any lock in the Linux kernel.Yes, although in last year, people try to convert these unlocked reads to READ_ONCE() or similar as otherwise the compiler is apparently allowed to generate code which is not safe. But that's a different story.
Is this ongoing work?Using such a macro would a) make our work much easier as we can instrument them, and b) would tell less experienced developers that no locking is needed.
Does the usage of READ_ONCE() imply that no lock is needed?Otherwise, one could introduce another macro for jbd2, such as #define READ_UNLOCKED() READ_ONCE(), which is more precise.
Also note
that although reading that particular word may be safe without any other locks, the lock still may be needed to safely interpret the value in the context of other fetched values (e.g., due to consistency among multiplestructure members).
Just a side quest: Do you have an example for such a situation? So sometimes requiring the lock is just the least
Yeah, I'm fine with that. Does this rule apply for the other members of journal_t (and transaction_t?) listed above?problematic solution - there's always the tradeoff between the speed and simplicity.All of the above members have word size, i.e., int, long, or ptr. Is it therefore safe to split the locking documentation as follows? @j_flags: General journaling state flags [r:nolocks, w:j_state_lock]I've checked the code and we usually use unlocked reads for quick, possibly racy checks and if they indicate we may need to do something then take the lock and do a reliable check. This is quite common pattern, not sure how to best document this. Maybe like [j_state_lock, no lock for quick racy checks]?
The following members are not word-sizes. Our results also suggest that no locks are needed. Can the proposed change be applied to them as well? transaction_t - t_chp_stats: j_checkpoint_semWhere do we read t_chp_stats outside of a lock? j_list_lock seems to be used pretty consistently there. Except perhaps __jbd2_journal_remove_checkpoint() but there we know we are already the only ones touching the transaction and thus its statistics.
I'm sorry. That's my mistake. There's no access without a lock.
jbd2_inode: - i_list: j_list_lockAnd here as well. I would not complicate the locking description unless we really have places that access these fields without locks...
Same here. - Alex
Honza
-- Technische Universität Dortmund Alexander Lochmann PGP key: 0xBC3EF6FD Otto-Hahn-Str. 16 phone: +49.231.7556141 D-44227 Dortmund fax: +49.231.7556116 http://ess.cs.tu-dortmund.de/Staff/al
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
