| Message ID | 20200428205903.61704-4-urezki@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce kvfree_rcu(1 or 2 arguments) | expand |
On Tue, Apr 28, 2020 at 10:58:42PM +0200, Uladzislau Rezki (Sony) wrote: > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > Simple clean up of comments in kfree_rcu() code to keep it consistent > with majority of commenting styles. > > Reviewed-by: Uladzislau Rezki <urezki@gmail.com> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> Hmmm... Exactly why is three additional characters per line preferable? Or in the case of block comments, either one or two additional lines, depending on /* */ style? I am (slowly) moving RCU to "//" for those reasons. ;-) Thanx, Paul > --- > kernel/rcu/tree.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index cd61649e1b00..1487af8e11e8 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3043,15 +3043,15 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp) > static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp, > unsigned long flags) > { > - // Attempt to start a new batch. > + /* Attempt to start a new batch. */ > krcp->monitor_todo = false; > if (queue_kfree_rcu_work(krcp)) { > - // Success! Our job is done here. > + /* Success! Our job is done here. */ > raw_spin_unlock_irqrestore(&krcp->lock, flags); > return; > } > > - // Previous RCU batch still in progress, try again later. > + /* Previous RCU batch still in progress, try again later. */ > krcp->monitor_todo = true; > schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES); > raw_spin_unlock_irqrestore(&krcp->lock, flags); > @@ -3151,14 +3151,14 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > unsigned long flags; > struct kfree_rcu_cpu *krcp; > > - local_irq_save(flags); // For safely calling this_cpu_ptr(). > + local_irq_save(flags); /* For safely calling this_cpu_ptr(). */ > krcp = this_cpu_ptr(&krc); > if (krcp->initialized) > raw_spin_lock(&krcp->lock); > > - // Queue the object but don't yet schedule the batch. > + /* Queue the object but don't yet schedule the batch. */ > if (debug_rcu_head_queue(head)) { > - // Probable double kfree_rcu(), just leak. > + /* Probable double kfree_rcu(), just leak. */ > WARN_ONCE(1, "%s(): Double-freed call. rcu_head %p\n", > __func__, head); > goto unlock_return; > @@ -3176,7 +3176,7 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > WRITE_ONCE(krcp->count, krcp->count + 1); > > - // Set timer to drain after KFREE_DRAIN_JIFFIES. > + /* Set timer to drain after KFREE_DRAIN_JIFFIES. */ > if (rcu_scheduler_active == RCU_SCHEDULER_RUNNING && > !krcp->monitor_todo) { > krcp->monitor_todo = true; > @@ -3722,7 +3722,7 @@ int rcutree_offline_cpu(unsigned int cpu) > > rcutree_affinity_setting(cpu, cpu); > > - // nohz_full CPUs need the tick for stop-machine to work quickly > + /* nohz_full CPUs need the tick for stop-machine to work quickly */ > tick_dep_set(TICK_DEP_BIT_RCU); > return 0; > } > -- > 2.20.1 >
On Fri, 2020-05-01 at 12:05 -0700, Paul E. McKenney wrote: > On Tue, Apr 28, 2020 at 10:58:42PM +0200, Uladzislau Rezki (Sony) wrote: > > Simple clean up of comments in kfree_rcu() code to keep it consistent > > with majority of commenting styles. [] > on /* */ style? > > I am (slowly) moving RCU to "//" for those reasons. ;-) I hope c99 comment styles are more commonly used soon too. checkpatch doesn't care. Perhaps a change to coding-style.rst --- Documentation/process/coding-style.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst index acb2f1b..fee647 100644 --- a/Documentation/process/coding-style.rst +++ b/Documentation/process/coding-style.rst @@ -565,6 +565,11 @@ comments is a little different. * but there is no initial almost-blank line. */ +.. code-block:: c + + // Single line and inline comments may also use the c99 // style + // Block comments as well + It's also important to comment data, whether they are basic types or derived types. To this end, use just one data declaration per line (no commas for multiple data declarations). This leaves you room for a small comment on each
On Fri, May 01, 2020 at 01:52:46PM -0700, Joe Perches wrote: > On Fri, 2020-05-01 at 12:05 -0700, Paul E. McKenney wrote: > > On Tue, Apr 28, 2020 at 10:58:42PM +0200, Uladzislau Rezki (Sony) wrote: > > > Simple clean up of comments in kfree_rcu() code to keep it consistent > > > with majority of commenting styles. > [] > > on /* */ style? > > > > I am (slowly) moving RCU to "//" for those reasons. ;-) > > I hope c99 comment styles are more commonly used soon too. > checkpatch doesn't care. > > Perhaps a change to coding-style.rst > --- > Documentation/process/coding-style.rst | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst > index acb2f1b..fee647 100644 > --- a/Documentation/process/coding-style.rst > +++ b/Documentation/process/coding-style.rst > @@ -565,6 +565,11 @@ comments is a little different. > * but there is no initial almost-blank line. > */ > > +.. code-block:: c > + > + // Single line and inline comments may also use the c99 // style > + // Block comments as well > + > It's also important to comment data, whether they are basic types or derived > types. To this end, use just one data declaration per line (no commas for > multiple data declarations). This leaves you room for a small comment on each > > Yeah that's fine with me. This patch just tries to keep it consistent. I am Ok with either style. thanks, - Joel
On Fri, May 01, 2020 at 12:05:55PM -0700, Paul E. McKenney wrote: > On Tue, Apr 28, 2020 at 10:58:42PM +0200, Uladzislau Rezki (Sony) wrote: > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > Simple clean up of comments in kfree_rcu() code to keep it consistent > > with majority of commenting styles. > > > > Reviewed-by: Uladzislau Rezki <urezki@gmail.com> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > Hmmm... > > Exactly why is three additional characters per line preferable? Or in > the case of block comments, either one or two additional lines, depending > on /* */ style? I prefer to keep the code consistent and then bulk convert it later. Its a bit ugly to read when its mixed up with "//" and "/* */" right now. We can convert it to // all at once later but until then it'll be good to keep it consistent in this file IMO. When I checked the kfree_rcu() code, it had more "/* */" than not, so this small change is less churn for now. thanks, - Joel > > I am (slowly) moving RCU to "//" for those reasons. ;-) > > Thanx, Paul > > > --- > > kernel/rcu/tree.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index cd61649e1b00..1487af8e11e8 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -3043,15 +3043,15 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp) > > static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp, > > unsigned long flags) > > { > > - // Attempt to start a new batch. > > + /* Attempt to start a new batch. */ > > krcp->monitor_todo = false; > > if (queue_kfree_rcu_work(krcp)) { > > - // Success! Our job is done here. > > + /* Success! Our job is done here. */ > > raw_spin_unlock_irqrestore(&krcp->lock, flags); > > return; > > } > > > > - // Previous RCU batch still in progress, try again later. > > + /* Previous RCU batch still in progress, try again later. */ > > krcp->monitor_todo = true; > > schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES); > > raw_spin_unlock_irqrestore(&krcp->lock, flags); > > @@ -3151,14 +3151,14 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > unsigned long flags; > > struct kfree_rcu_cpu *krcp; > > > > - local_irq_save(flags); // For safely calling this_cpu_ptr(). > > + local_irq_save(flags); /* For safely calling this_cpu_ptr(). */ > > krcp = this_cpu_ptr(&krc); > > if (krcp->initialized) > > raw_spin_lock(&krcp->lock); > > > > - // Queue the object but don't yet schedule the batch. > > + /* Queue the object but don't yet schedule the batch. */ > > if (debug_rcu_head_queue(head)) { > > - // Probable double kfree_rcu(), just leak. > > + /* Probable double kfree_rcu(), just leak. */ > > WARN_ONCE(1, "%s(): Double-freed call. rcu_head %p\n", > > __func__, head); > > goto unlock_return; > > @@ -3176,7 +3176,7 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > > > WRITE_ONCE(krcp->count, krcp->count + 1); > > > > - // Set timer to drain after KFREE_DRAIN_JIFFIES. > > + /* Set timer to drain after KFREE_DRAIN_JIFFIES. */ > > if (rcu_scheduler_active == RCU_SCHEDULER_RUNNING && > > !krcp->monitor_todo) { > > krcp->monitor_todo = true; > > @@ -3722,7 +3722,7 @@ int rcutree_offline_cpu(unsigned int cpu) > > > > rcutree_affinity_setting(cpu, cpu); > > > > - // nohz_full CPUs need the tick for stop-machine to work quickly > > + /* nohz_full CPUs need the tick for stop-machine to work quickly */ > > tick_dep_set(TICK_DEP_BIT_RCU); > > return 0; > > } > > -- > > 2.20.1 > >
On Sun, May 03, 2020 at 07:44:00PM -0400, Joel Fernandes wrote: > On Fri, May 01, 2020 at 01:52:46PM -0700, Joe Perches wrote: > > On Fri, 2020-05-01 at 12:05 -0700, Paul E. McKenney wrote: > > > On Tue, Apr 28, 2020 at 10:58:42PM +0200, Uladzislau Rezki (Sony) wrote: > > > > Simple clean up of comments in kfree_rcu() code to keep it consistent > > > > with majority of commenting styles. > > [] > > > on /* */ style? > > > > > > I am (slowly) moving RCU to "//" for those reasons. ;-) > > > > I hope c99 comment styles are more commonly used soon too. > > checkpatch doesn't care. > > > > Perhaps a change to coding-style.rst > > --- > > Documentation/process/coding-style.rst | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst > > index acb2f1b..fee647 100644 > > --- a/Documentation/process/coding-style.rst > > +++ b/Documentation/process/coding-style.rst > > @@ -565,6 +565,11 @@ comments is a little different. > > * but there is no initial almost-blank line. > > */ > > > > +.. code-block:: c > > + > > + // Single line and inline comments may also use the c99 // style > > + // Block comments as well > > + > > It's also important to comment data, whether they are basic types or derived > > types. To this end, use just one data declaration per line (no commas for > > multiple data declarations). This leaves you room for a small comment on each > > Yeah that's fine with me. This patch just tries to keep it consistent. I am > Ok with either style. My approach has been gradual change. Big-bang changes of this sort cause quite a bit of trouble. So I use "//" in new code and (sometimes) convert nearby ones when making a change. Thanx, Paul
On Sun, May 03, 2020 at 07:52:13PM -0400, Joel Fernandes wrote: > On Fri, May 01, 2020 at 12:05:55PM -0700, Paul E. McKenney wrote: > > On Tue, Apr 28, 2020 at 10:58:42PM +0200, Uladzislau Rezki (Sony) wrote: > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > > > Simple clean up of comments in kfree_rcu() code to keep it consistent > > > with majority of commenting styles. > > > > > > Reviewed-by: Uladzislau Rezki <urezki@gmail.com> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > > > Hmmm... > > > > Exactly why is three additional characters per line preferable? Or in > > the case of block comments, either one or two additional lines, depending > > on /* */ style? > > I prefer to keep the code consistent and then bulk convert it later. Its a > bit ugly to read when its mixed up with "//" and "/* */" right now. We can > convert it to // all at once later but until then it'll be good to keep it > consistent in this file IMO. When I checked the kfree_rcu() code, it had more > "/* */" than not, so this small change is less churn for now. Please just drop this patch along with the other "//"-to-"/* */" regressions. If you want to convert more comments to "//" within the confines of the kfree_rcu() code, I am probably OK with that. But again, a big-bang change of this sort often causes problems due to lots of potential rebase/merge conflicts. Thanx, Paul > thanks, > > - Joel > > > > > I am (slowly) moving RCU to "//" for those reasons. ;-) > > > > Thanx, Paul > > > > > --- > > > kernel/rcu/tree.c | 16 ++++++++-------- > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index cd61649e1b00..1487af8e11e8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3043,15 +3043,15 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp) > > > static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp, > > > unsigned long flags) > > > { > > > - // Attempt to start a new batch. > > > + /* Attempt to start a new batch. */ > > > krcp->monitor_todo = false; > > > if (queue_kfree_rcu_work(krcp)) { > > > - // Success! Our job is done here. > > > + /* Success! Our job is done here. */ > > > raw_spin_unlock_irqrestore(&krcp->lock, flags); > > > return; > > > } > > > > > > - // Previous RCU batch still in progress, try again later. > > > + /* Previous RCU batch still in progress, try again later. */ > > > krcp->monitor_todo = true; > > > schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES); > > > raw_spin_unlock_irqrestore(&krcp->lock, flags); > > > @@ -3151,14 +3151,14 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > > unsigned long flags; > > > struct kfree_rcu_cpu *krcp; > > > > > > - local_irq_save(flags); // For safely calling this_cpu_ptr(). > > > + local_irq_save(flags); /* For safely calling this_cpu_ptr(). */ > > > krcp = this_cpu_ptr(&krc); > > > if (krcp->initialized) > > > raw_spin_lock(&krcp->lock); > > > > > > - // Queue the object but don't yet schedule the batch. > > > + /* Queue the object but don't yet schedule the batch. */ > > > if (debug_rcu_head_queue(head)) { > > > - // Probable double kfree_rcu(), just leak. > > > + /* Probable double kfree_rcu(), just leak. */ > > > WARN_ONCE(1, "%s(): Double-freed call. rcu_head %p\n", > > > __func__, head); > > > goto unlock_return; > > > @@ -3176,7 +3176,7 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > > > > > WRITE_ONCE(krcp->count, krcp->count + 1); > > > > > > - // Set timer to drain after KFREE_DRAIN_JIFFIES. > > > + /* Set timer to drain after KFREE_DRAIN_JIFFIES. */ > > > if (rcu_scheduler_active == RCU_SCHEDULER_RUNNING && > > > !krcp->monitor_todo) { > > > krcp->monitor_todo = true; > > > @@ -3722,7 +3722,7 @@ int rcutree_offline_cpu(unsigned int cpu) > > > > > > rcutree_affinity_setting(cpu, cpu); > > > > > > - // nohz_full CPUs need the tick for stop-machine to work quickly > > > + /* nohz_full CPUs need the tick for stop-machine to work quickly */ > > > tick_dep_set(TICK_DEP_BIT_RCU); > > > return 0; > > > } > > > -- > > > 2.20.1 > > >
On Sun, 2020-05-03 at 17:23 -0700, Paul E. McKenney wrote: > On Sun, May 03, 2020 at 07:44:00PM -0400, Joel Fernandes wrote: > > On Fri, May 01, 2020 at 01:52:46PM -0700, Joe Perches wrote: [] > > > Perhaps a change to coding-style.rst > > > --- > > > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst [] > > > @@ -565,6 +565,11 @@ comments is a little different. > > > * but there is no initial almost-blank line. > > > */ > > > > > > +.. code-block:: c > > > + > > > + // Single line and inline comments may also use the c99 // style > > > + // Block comments as well > > > + > > > It's also important to comment data, whether they are basic types or derived > > > types. To this end, use just one data declaration per line (no commas for > > > multiple data declarations). This leaves you room for a small comment on each > > > > Yeah that's fine with me. This patch just tries to keep it consistent. I am > > Ok with either style. > > My approach has been gradual change. Big-bang changes of this sort > cause quite a bit of trouble. So I use "//" in new code and (sometimes) > convert nearby ones when making a change. I think that's good too. Mixing styles in the same compilation unit is not generally the right thing to do. But right now, c99 comments are not specified as allowed in coding-style so it's likely appropriate to add something like this there.
On Sun, May 03, 2020 at 05:26:56PM -0700, Paul E. McKenney wrote: > On Sun, May 03, 2020 at 07:52:13PM -0400, Joel Fernandes wrote: > > On Fri, May 01, 2020 at 12:05:55PM -0700, Paul E. McKenney wrote: > > > On Tue, Apr 28, 2020 at 10:58:42PM +0200, Uladzislau Rezki (Sony) wrote: > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > > > > > Simple clean up of comments in kfree_rcu() code to keep it consistent > > > > with majority of commenting styles. > > > > > > > > Reviewed-by: Uladzislau Rezki <urezki@gmail.com> > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > > > > > Hmmm... > > > > > > Exactly why is three additional characters per line preferable? Or in > > > the case of block comments, either one or two additional lines, depending > > > on /* */ style? > > > > I prefer to keep the code consistent and then bulk convert it later. Its a > > bit ugly to read when its mixed up with "//" and "/* */" right now. We can > > convert it to // all at once later but until then it'll be good to keep it > > consistent in this file IMO. When I checked the kfree_rcu() code, it had more > > "/* */" than not, so this small change is less churn for now. > > Please just drop this patch along with the other "//"-to-"/* */" > regressions. Right now in your rcu/dev branch (without applying this series),in kfree_rcu_drain_unlock() and the functions before and after it, it is inconsistent. Also in kfree_call_rcu(), it is: // Queue the object but don't yet schedule the batch. if (debug_rcu_head_queue(head)) { // Probable double kfree_rcu(), just leak. WARN_ONCE(1, "%s(): Double-freed call. rcu_head %p\n", __func__, head); goto unlock_return; } /* * Under high memory pressure GFP_NOWAIT can fail, * in that case the emergency path is maintained. */ > If you want to convert more comments to "//" within the confines of the > kfree_rcu() code, I am probably OK with that. But again, a big-bang > change of this sort often causes problems due to lots of potential > rebase/merge conflicts. Ok. Since this series touched kfree-related RCU code, converting all of the kfree-related RCU code to "//" is Ok with me. Just wanted to keep it consistent :) thanks, - Joel > > Thanx, Paul > > > thanks, > > > > - Joel > > > > > > > > I am (slowly) moving RCU to "//" for those reasons. ;-) > > > > > > Thanx, Paul > > > > > > > --- > > > > kernel/rcu/tree.c | 16 ++++++++-------- > > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > index cd61649e1b00..1487af8e11e8 100644 > > > > --- a/kernel/rcu/tree.c > > > > +++ b/kernel/rcu/tree.c > > > > @@ -3043,15 +3043,15 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp) > > > > static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp, > > > > unsigned long flags) > > > > { > > > > - // Attempt to start a new batch. > > > > + /* Attempt to start a new batch. */ > > > > krcp->monitor_todo = false; > > > > if (queue_kfree_rcu_work(krcp)) { > > > > - // Success! Our job is done here. > > > > + /* Success! Our job is done here. */ > > > > raw_spin_unlock_irqrestore(&krcp->lock, flags); > > > > return; > > > > } > > > > > > > > - // Previous RCU batch still in progress, try again later. > > > > + /* Previous RCU batch still in progress, try again later. */ > > > > krcp->monitor_todo = true; > > > > schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES); > > > > raw_spin_unlock_irqrestore(&krcp->lock, flags); > > > > @@ -3151,14 +3151,14 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > > > unsigned long flags; > > > > struct kfree_rcu_cpu *krcp; > > > > > > > > - local_irq_save(flags); // For safely calling this_cpu_ptr(). > > > > + local_irq_save(flags); /* For safely calling this_cpu_ptr(). */ > > > > krcp = this_cpu_ptr(&krc); > > > > if (krcp->initialized) > > > > raw_spin_lock(&krcp->lock); > > > > > > > > - // Queue the object but don't yet schedule the batch. > > > > + /* Queue the object but don't yet schedule the batch. */ > > > > if (debug_rcu_head_queue(head)) { > > > > - // Probable double kfree_rcu(), just leak. > > > > + /* Probable double kfree_rcu(), just leak. */ > > > > WARN_ONCE(1, "%s(): Double-freed call. rcu_head %p\n", > > > > __func__, head); > > > > goto unlock_return; > > > > @@ -3176,7 +3176,7 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > > > > > > > WRITE_ONCE(krcp->count, krcp->count + 1); > > > > > > > > - // Set timer to drain after KFREE_DRAIN_JIFFIES. > > > > + /* Set timer to drain after KFREE_DRAIN_JIFFIES. */ > > > > if (rcu_scheduler_active == RCU_SCHEDULER_RUNNING && > > > > !krcp->monitor_todo) { > > > > krcp->monitor_todo = true; > > > > @@ -3722,7 +3722,7 @@ int rcutree_offline_cpu(unsigned int cpu) > > > > > > > > rcutree_affinity_setting(cpu, cpu); > > > > > > > > - // nohz_full CPUs need the tick for stop-machine to work quickly > > > > + /* nohz_full CPUs need the tick for stop-machine to work quickly */ > > > > tick_dep_set(TICK_DEP_BIT_RCU); > > > > return 0; > > > > } > > > > -- > > > > 2.20.1 > > > >
On Sun, May 03, 2020 at 05:23:09PM -0700, Paul E. McKenney wrote: > On Sun, May 03, 2020 at 07:44:00PM -0400, Joel Fernandes wrote: > > On Fri, May 01, 2020 at 01:52:46PM -0700, Joe Perches wrote: > > > On Fri, 2020-05-01 at 12:05 -0700, Paul E. McKenney wrote: > > > > On Tue, Apr 28, 2020 at 10:58:42PM +0200, Uladzislau Rezki (Sony) wrote: > > > > > Simple clean up of comments in kfree_rcu() code to keep it consistent > > > > > with majority of commenting styles. > > > [] > > > > on /* */ style? > > > > > > > > I am (slowly) moving RCU to "//" for those reasons. ;-) > > > > > > I hope c99 comment styles are more commonly used soon too. > > > checkpatch doesn't care. > > > > > > Perhaps a change to coding-style.rst > > > --- > > > Documentation/process/coding-style.rst | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst > > > index acb2f1b..fee647 100644 > > > --- a/Documentation/process/coding-style.rst > > > +++ b/Documentation/process/coding-style.rst > > > @@ -565,6 +565,11 @@ comments is a little different. > > > * but there is no initial almost-blank line. > > > */ > > > > > > +.. code-block:: c > > > + > > > + // Single line and inline comments may also use the c99 // style > > > + // Block comments as well > > > + > > > It's also important to comment data, whether they are basic types or derived > > > types. To this end, use just one data declaration per line (no commas for > > > multiple data declarations). This leaves you room for a small comment on each > > > > Yeah that's fine with me. This patch just tries to keep it consistent. I am > > Ok with either style. > > My approach has been gradual change. Big-bang changes of this sort > cause quite a bit of trouble. So I use "//" in new code and (sometimes) > convert nearby ones when making a change. Ok thanks for the guidance on that, will follow similar conversion strategy as well. thanks, - Joel
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index cd61649e1b00..1487af8e11e8 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3043,15 +3043,15 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp) static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp, unsigned long flags) { - // Attempt to start a new batch. + /* Attempt to start a new batch. */ krcp->monitor_todo = false; if (queue_kfree_rcu_work(krcp)) { - // Success! Our job is done here. + /* Success! Our job is done here. */ raw_spin_unlock_irqrestore(&krcp->lock, flags); return; } - // Previous RCU batch still in progress, try again later. + /* Previous RCU batch still in progress, try again later. */ krcp->monitor_todo = true; schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES); raw_spin_unlock_irqrestore(&krcp->lock, flags); @@ -3151,14 +3151,14 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) unsigned long flags; struct kfree_rcu_cpu *krcp; - local_irq_save(flags); // For safely calling this_cpu_ptr(). + local_irq_save(flags); /* For safely calling this_cpu_ptr(). */ krcp = this_cpu_ptr(&krc); if (krcp->initialized) raw_spin_lock(&krcp->lock); - // Queue the object but don't yet schedule the batch. + /* Queue the object but don't yet schedule the batch. */ if (debug_rcu_head_queue(head)) { - // Probable double kfree_rcu(), just leak. + /* Probable double kfree_rcu(), just leak. */ WARN_ONCE(1, "%s(): Double-freed call. rcu_head %p\n", __func__, head); goto unlock_return; @@ -3176,7 +3176,7 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) WRITE_ONCE(krcp->count, krcp->count + 1); - // Set timer to drain after KFREE_DRAIN_JIFFIES. + /* Set timer to drain after KFREE_DRAIN_JIFFIES. */ if (rcu_scheduler_active == RCU_SCHEDULER_RUNNING && !krcp->monitor_todo) { krcp->monitor_todo = true; @@ -3722,7 +3722,7 @@ int rcutree_offline_cpu(unsigned int cpu) rcutree_affinity_setting(cpu, cpu); - // nohz_full CPUs need the tick for stop-machine to work quickly + /* nohz_full CPUs need the tick for stop-machine to work quickly */ tick_dep_set(TICK_DEP_BIT_RCU); return 0; }