| Message ID | 20190408083500.66759-1-alexander.lochmann@tu-dortmund.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] Updated locking documentation for transaction_t | expand |
Hi Ted, Have you had the chance to review the most recent version of the patch? Does it look reasonable to you? Cheers, Alex On 08.04.19 10:35, Alexander Lochmann wrote: > We used LockDoc to derive locking rules for each member > of struct transaction_t. > Based on those results, we extended the existing documentation > by more members of struct transaction_t, and updated the existing > documentation. > > Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de> > Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de> > --- > include/linux/jbd2.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 0f919d5fe84f..34b728e2b702 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -534,6 +534,7 @@ struct transaction_chp_stats_s { > * The transaction keeps track of all of the buffers modified by a > * running transaction, and all of the buffers committed but not yet > * flushed to home for finished transactions. > + * (Locking Documentation improved by LockDoc) > */ > > /* > @@ -652,12 +653,12 @@ struct transaction_s > unsigned long t_start; > > /* > - * When commit was requested > + * When commit was requested [journal_t.j_state_lock] > */ > unsigned long t_requested; > > /* > - * Checkpointing stats [j_checkpoint_sem] > + * Checkpointing stats [journal_t.j_list_lock] > */ > struct transaction_chp_stats_s t_chp_stats; > >
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. 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] 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_sem jbd2_inode: - i_list: j_list_lock Cheers, Alex
Hi Ted, hi Jack, have you had the chance to review our results? Cheers, Alex 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. > 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] > > 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_sem > jbd2_inode: > - i_list: j_list_lock > > Cheers, > Alex >
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. 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 multiple structure members). So sometimes requiring the lock is just the least 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_sem Where 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. > > jbd2_inode: > > - i_list: j_list_lock And here as well. I would not complicate the locking description unless we really have places that access these fields without locks... Honza
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 multiple > structure members). Just a side quest: Do you have an example for such a situation? So sometimes requiring the lock is just the least > 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]? > Yeah, I'm fine with that. Does this rule apply for the other members of journal_t (and transaction_t?) listed above? >>> 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_sem > > Where 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_lock > > And 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 >
On Tue 09-02-21 10:58:48, Alexander Lochmann wrote: > > > 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? Yes, in a way. It's mostly prompted by KCSAN warnings generated by syzbot ;). > 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. Yes, I agree that it has some benefit for documentation and automatic checkers as well. OTOH code readability is sometimes hurt by this... > Does the usage of READ_ONCE() imply that no lock is needed? No, but it does indicate there's something unusual happening with the variable - usually that variable write can race with this read. > Otherwise, one could introduce another macro for jbd2, such as #define > READ_UNLOCKED() READ_ONCE(), which is more precise. Well, yes, but OTOH special macros for small subsystems like this are making more harm than good in terms of readability since people have to lookup what exactly they mean anyway. > 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 multiple > > structure members). > Just a side quest: Do you have an example for such a situation? Definitely. The simplest case is: You can fetch journal->j_running_transaction pointer any time without any problem. But you can *dereference* it only if you hold the j_state_lock while fetching the pointer and dereferencing it. > So sometimes requiring the lock is just the least > > 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]? > > > Yeah, I'm fine with that. Does this rule apply for the other members of > journal_t (and transaction_t?) listed above? Yes. Honza
On 09.02.21 13:00, Jan Kara wrote: >>> 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? > > Yes, in a way. It's mostly prompted by KCSAN warnings generated by syzbot > ;). > >> 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. > > Yes, I agree that it has some benefit for documentation and automatic > checkers as well. OTOH code readability is sometimes hurt by this... > >> Does the usage of READ_ONCE() imply that no lock is needed? > > No, but it does indicate there's something unusual happening with the > variable - usually that variable write can race with this read. > >> Otherwise, one could introduce another macro for jbd2, such as #define >> READ_UNLOCKED() READ_ONCE(), which is more precise. > > Well, yes, but OTOH special macros for small subsystems like this are making > more harm than good in terms of readability since people have to lookup > what exactly they mean anyway. So the only option left would be a global macro such as READ_ONCE() I guess. How hard would it be to establish such a global notation? It would make things a lot easier for LockDoc, because we can instrument such a macro, and therefore can annotate those accesses.> > Definitely. The simplest case is: You can fetch > journal->j_running_transaction pointer any time without any problem. But > you can *dereference* it only if you hold the j_state_lock while fetching the > pointer and dereferencing it. Thx. > >> So sometimes requiring the lock is just the least >>> 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]? >>> >> Yeah, I'm fine with that. Does this rule apply for the other members of >> journal_t (and transaction_t?) listed above? > > Yes. Thx. I'll submit a patch for those elements. For now, this will improve LockDoc's results as we can add "no locks needed" to our config for j_flags. We check whether the observed accesses match the documented locking rules. LockDoc will accept both results "j_list_lock" and "no locks needed" for reading j_flags. However, real faulty unlocked accesses will be concealed. :-( - Alex > > Honza >
On Tue 09-02-21 14:47:28, Alexander Lochmann wrote: > On 09.02.21 13:00, Jan Kara wrote: > > > > 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? > > > > Yes, in a way. It's mostly prompted by KCSAN warnings generated by syzbot > > ;). > > > > > 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. > > > > Yes, I agree that it has some benefit for documentation and automatic > > checkers as well. OTOH code readability is sometimes hurt by this... > > > > > Does the usage of READ_ONCE() imply that no lock is needed? > > > > No, but it does indicate there's something unusual happening with the > > variable - usually that variable write can race with this read. > > > > > Otherwise, one could introduce another macro for jbd2, such as #define > > > READ_UNLOCKED() READ_ONCE(), which is more precise. > > > > Well, yes, but OTOH special macros for small subsystems like this are > > making more harm than good in terms of readability since people have to > > lookup what exactly they mean anyway. > > So the only option left would be a global macro such as READ_ONCE() I guess. > How hard would it be to establish such a global notation? > It would make things a lot easier for LockDoc, because we can instrument > such a macro, and therefore can annotate those accesses.> Well, READ_ONCE() macro already exists in the kernel and is used in quite some places. Generally, there's concensus that newly added unlocked accesses should use that macro. But the amount of already existing unlocked accesses in the kernel is large and the problem is mostly theoretical / for machine checkers so there's some resistance to the churn generated by global search-and-replace approach. But as I said we are moving in that direction and eventually get there. Honza
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 0f919d5fe84f..34b728e2b702 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -534,6 +534,7 @@ struct transaction_chp_stats_s { * The transaction keeps track of all of the buffers modified by a * running transaction, and all of the buffers committed but not yet * flushed to home for finished transactions. + * (Locking Documentation improved by LockDoc) */ /* @@ -652,12 +653,12 @@ struct transaction_s unsigned long t_start; /* - * When commit was requested + * When commit was requested [journal_t.j_state_lock] */ unsigned long t_requested; /* - * Checkpointing stats [j_checkpoint_sem] + * Checkpointing stats [journal_t.j_list_lock] */ struct transaction_chp_stats_s t_chp_stats;