| Message ID | 20220128131006.67712-4-michel@lespinasse.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Speculative page faults | expand |
* Michel Lespinasse <michel@lespinasse.org> [220128 08:10]: > In the mmap locking API, the *_killable() functions return an error > (or 0 on success), and the *_trylock() functions return a boolean > (true on success). > > Rename the return values "int error" and "bool ok", respectively, > rather than using "ret" for both cases which I find less readable. Would it be better to add function documentation in regards to return types? I think changing the variables does help, but putting a block with Return: <what's above> would work best. > > Signed-off-by: Michel Lespinasse <michel@lespinasse.org> > --- > include/linux/mmap_lock.h | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h > index db9785e11274..1b14468183d7 100644 > --- a/include/linux/mmap_lock.h > +++ b/include/linux/mmap_lock.h > @@ -81,22 +81,22 @@ static inline void mmap_write_lock_nested(struct mm_struct *mm, int subclass) > > static inline int mmap_write_lock_killable(struct mm_struct *mm) > { > - int ret; > + int error; > > __mmap_lock_trace_start_locking(mm, true); > - ret = down_write_killable(&mm->mmap_lock); > - __mmap_lock_trace_acquire_returned(mm, true, ret == 0); > - return ret; > + error = down_write_killable(&mm->mmap_lock); > + __mmap_lock_trace_acquire_returned(mm, true, !error); > + return error; > } > > static inline bool mmap_write_trylock(struct mm_struct *mm) > { > - bool ret; > + bool ok; > > __mmap_lock_trace_start_locking(mm, true); > - ret = down_write_trylock(&mm->mmap_lock) != 0; > - __mmap_lock_trace_acquire_returned(mm, true, ret); > - return ret; > + ok = down_write_trylock(&mm->mmap_lock) != 0; > + __mmap_lock_trace_acquire_returned(mm, true, ok); > + return ok; > } > > static inline void mmap_write_unlock(struct mm_struct *mm) > @@ -120,22 +120,22 @@ static inline void mmap_read_lock(struct mm_struct *mm) > > static inline int mmap_read_lock_killable(struct mm_struct *mm) > { > - int ret; > + int error; > > __mmap_lock_trace_start_locking(mm, false); > - ret = down_read_killable(&mm->mmap_lock); > - __mmap_lock_trace_acquire_returned(mm, false, ret == 0); > - return ret; > + error = down_read_killable(&mm->mmap_lock); > + __mmap_lock_trace_acquire_returned(mm, false, !error); > + return error; > } > > static inline bool mmap_read_trylock(struct mm_struct *mm) > { > - bool ret; > + bool ok; > > __mmap_lock_trace_start_locking(mm, false); > - ret = down_read_trylock(&mm->mmap_lock) != 0; > - __mmap_lock_trace_acquire_returned(mm, false, ret); > - return ret; > + ok = down_read_trylock(&mm->mmap_lock) != 0; > + __mmap_lock_trace_acquire_returned(mm, false, ok); > + return ok; > } > > static inline void mmap_read_unlock(struct mm_struct *mm) > -- > 2.20.1 >
On Mon, Jan 31, 2022 at 04:17:43PM +0000, Liam Howlett wrote: > * Michel Lespinasse <michel@lespinasse.org> [220128 08:10]: > > In the mmap locking API, the *_killable() functions return an error > > (or 0 on success), and the *_trylock() functions return a boolean > > (true on success). > > > > Rename the return values "int error" and "bool ok", respectively, > > rather than using "ret" for both cases which I find less readable. > > Would it be better to add function documentation in regards to return > types? I think changing the variables does help, but putting a block > with Return: <what's above> would work best. That would work, I guess. I'm not sure what it says about our general coding style, that the comment would kinda stick out like a sore thumb compared to the rest of the file, or of similar include files (say, other lock definitions). I don't care very strongly either way. -- Michel "walken" Lespinasse
diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h index db9785e11274..1b14468183d7 100644 --- a/include/linux/mmap_lock.h +++ b/include/linux/mmap_lock.h @@ -81,22 +81,22 @@ static inline void mmap_write_lock_nested(struct mm_struct *mm, int subclass) static inline int mmap_write_lock_killable(struct mm_struct *mm) { - int ret; + int error; __mmap_lock_trace_start_locking(mm, true); - ret = down_write_killable(&mm->mmap_lock); - __mmap_lock_trace_acquire_returned(mm, true, ret == 0); - return ret; + error = down_write_killable(&mm->mmap_lock); + __mmap_lock_trace_acquire_returned(mm, true, !error); + return error; } static inline bool mmap_write_trylock(struct mm_struct *mm) { - bool ret; + bool ok; __mmap_lock_trace_start_locking(mm, true); - ret = down_write_trylock(&mm->mmap_lock) != 0; - __mmap_lock_trace_acquire_returned(mm, true, ret); - return ret; + ok = down_write_trylock(&mm->mmap_lock) != 0; + __mmap_lock_trace_acquire_returned(mm, true, ok); + return ok; } static inline void mmap_write_unlock(struct mm_struct *mm) @@ -120,22 +120,22 @@ static inline void mmap_read_lock(struct mm_struct *mm) static inline int mmap_read_lock_killable(struct mm_struct *mm) { - int ret; + int error; __mmap_lock_trace_start_locking(mm, false); - ret = down_read_killable(&mm->mmap_lock); - __mmap_lock_trace_acquire_returned(mm, false, ret == 0); - return ret; + error = down_read_killable(&mm->mmap_lock); + __mmap_lock_trace_acquire_returned(mm, false, !error); + return error; } static inline bool mmap_read_trylock(struct mm_struct *mm) { - bool ret; + bool ok; __mmap_lock_trace_start_locking(mm, false); - ret = down_read_trylock(&mm->mmap_lock) != 0; - __mmap_lock_trace_acquire_returned(mm, false, ret); - return ret; + ok = down_read_trylock(&mm->mmap_lock) != 0; + __mmap_lock_trace_acquire_returned(mm, false, ok); + return ok; } static inline void mmap_read_unlock(struct mm_struct *mm)
In the mmap locking API, the *_killable() functions return an error (or 0 on success), and the *_trylock() functions return a boolean (true on success). Rename the return values "int error" and "bool ok", respectively, rather than using "ret" for both cases which I find less readable. Signed-off-by: Michel Lespinasse <michel@lespinasse.org> --- include/linux/mmap_lock.h | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)