| Message ID | 20180323171736.bcyi2oiolge4l6hl@linutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, 23 Mar 2018 18:17:36 +0100 "bigeasy@linutronix.de" <bigeasy@linutronix.de> wrote: > There are a few functions which check for if the lock is held > (spin_lock_assert()) and the interrupts are disabled (irqs_disabled()). > >From looking at the code, each function is static, the caller is near by > and does spin_lock_irq|safe(). As Linus puts it: > > |It's not like this is some function that is exported to random users, > |and we should check that the calling convention is right. > | > |This looks like "it may have been useful during coding to document > |things, but it's not useful long-term". > > Remove those checks. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > drivers/target/target_core_tmr.c | 2 -- > drivers/target/target_core_transport.c | 6 ------ > 2 files changed, 8 deletions(-) > > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c > index 9c7bc1ca341a..3d35dad1de2c 100644 > --- a/drivers/target/target_core_tmr.c > +++ b/drivers/target/target_core_tmr.c Can you add a comment above the functions though? /* Expects to have se_cmd->se_sess->sess_cmd_lock held */ > @@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd, > { > struct se_session *sess = se_cmd->se_sess; > > - assert_spin_locked(&sess->sess_cmd_lock); > - WARN_ON_ONCE(!irqs_disabled()); > /* > * If command already reached CMD_T_COMPLETE state within > * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown, > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 74b646f165d4..2c3ddb3a4124 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c /* Expects to have cmd->t_state_lock held */ > @@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop, > __acquires(&cmd->t_state_lock) > { > > - assert_spin_locked(&cmd->t_state_lock); > - WARN_ON_ONCE(!irqs_disabled()); > - > if (fabric_stop) > cmd->transport_state |= CMD_T_FABRIC_STOP; > /* Expects to have cmd->t_state_lock held */ > @@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status) > { > int ret; > > - assert_spin_locked(&cmd->t_state_lock); > - WARN_ON_ONCE(!irqs_disabled()); > - > if (!(cmd->transport_state & CMD_T_ABORTED)) > return 0; > /* -- Steve
On 2018-03-26 11:13:59 [-0400], Steven Rostedt wrote: > > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c > > index 9c7bc1ca341a..3d35dad1de2c 100644 > > --- a/drivers/target/target_core_tmr.c > > +++ b/drivers/target/target_core_tmr.c > > Can you add a comment above the functions though? > > /* Expects to have se_cmd->se_sess->sess_cmd_lock held */ I could. I haven't heard from Bart / Nicholas about their opinion. I know, we do this other places but Bart was against this kind of annotation in 2/2 of this thread. So I am waiting to hear from them :) > > -- Steve Sebastian
On Wed, 2018-03-28 at 12:15 +0200, bigeasy@linutronix.de wrote: > On 2018-03-26 11:13:59 [-0400], Steven Rostedt wrote: > > > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c > > > index 9c7bc1ca341a..3d35dad1de2c 100644 > > > --- a/drivers/target/target_core_tmr.c > > > +++ b/drivers/target/target_core_tmr.c > > > > Can you add a comment above the functions though? > > > > /* Expects to have se_cmd->se_sess->sess_cmd_lock held */ > > I could. I haven't heard from Bart / Nicholas about their opinion. I > know, we do this other places but Bart was against this kind of > annotation in 2/2 of this thread. > So I am waiting to hear from them :) The names of the two functions touched by patch 1/2 start with a double underscore. That by itself is already a hint that these should be called with a lock held (I know that this is not a universal convention in the Linux kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2 with the above comment added. Bart.
On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote: > The names of the two functions touched by patch 1/2 start with a double > underscore. That by itself is already a hint that these should be called with > a lock held (I know that this is not a universal convention in the Linux > kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2 > with the above comment added. Okay. In that case let me update 1/2. But 2/2 with the comment as Steven suggested is still a no no for you? > Bart. Sebastian
On 03/28/18 08:14, bigeasy@linutronix.de wrote: > On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote: >> The names of the two functions touched by patch 1/2 start with a double >> underscore. That by itself is already a hint that these should be called with >> a lock held (I know that this is not a universal convention in the Linux >> kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2 >> with the above comment added. > > Okay. In that case let me update 1/2. > But 2/2 with the comment as Steven suggested is still a no no for you? Although I'm not enthusiast about patch 2/2, if others agree with that patch I'm fine with that patch being sent upstream. Thanks, Bart.
On 2018-03-28 08:31:38 [-0700], Bart Van Assche wrote: > On 03/28/18 08:14, bigeasy@linutronix.de wrote: > > On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote: > > > The names of the two functions touched by patch 1/2 start with a double > > > underscore. That by itself is already a hint that these should be called with > > > a lock held (I know that this is not a universal convention in the Linux > > > kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2 > > > with the above comment added. > > > > Okay. In that case let me update 1/2. > > But 2/2 with the comment as Steven suggested is still a no no for you? > > Although I'm not enthusiast about patch 2/2, if others agree with that patch > I'm fine with that patch being sent upstream. I've been waiting for something to happen but nobody replied. Bart, you were fine with 1/2 as posted but not too happy about 2/2. Assuming we keep 1/2 as is and I remove just the "WARN_ON_ONCE(!irqs_disabled());" from 2/2 (keeping the assert_spin_locked()), would that improve your mood? Lockdep would still perform full validation, yell if __transport_check_aborted_status() was invoked without locking and also yell abut missing IRQ-save at locking time of ->t_state_lock). > Thanks, > > Bart. Sebastian
On Mon, 2018-05-28 at 16:35 +0200, bigeasy@linutronix.de wrote: > On 2018-03-28 08:31:38 [-0700], Bart Van Assche wrote: > > On 03/28/18 08:14, bigeasy@linutronix.de wrote: > > > On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote: > > > > The names of the two functions touched by patch 1/2 start with a double > > > > underscore. That by itself is already a hint that these should be called with > > > > a lock held (I know that this is not a universal convention in the Linux > > > > kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2 > > > > with the above comment added. > > > > > > Okay. In that case let me update 1/2. > > > But 2/2 with the comment as Steven suggested is still a no no for you? > > > > Although I'm not enthusiast about patch 2/2, if others agree with that patch > > I'm fine with that patch being sent upstream. > > I've been waiting for something to happen but nobody replied. > Bart, you were fine with 1/2 as posted but not too happy about 2/2. > Assuming we keep 1/2 as is and I remove just the > "WARN_ON_ONCE(!irqs_disabled());" from 2/2 (keeping the > assert_spin_locked()), would that improve your mood? Lockdep would still > perform full validation, yell if __transport_check_aborted_status() was > invoked without locking and also yell abut missing IRQ-save at locking > time of ->t_state_lock). Would adding WARN_ON_ONCE(!irqs_disabled()) work fine with an RT kernel? Thanks, Bart.
On 2018-06-04 07:02:15 [+0000], Bart Van Assche wrote: > > Would adding WARN_ON_ONCE(!irqs_disabled()) work fine with an RT kernel? no, because the interrupts are not disabled at this point on RT. Lockdep works fine on RT. > Thanks, > > Bart. Sebastian
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c index 9c7bc1ca341a..3d35dad1de2c 100644 --- a/drivers/target/target_core_tmr.c +++ b/drivers/target/target_core_tmr.c @@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd, { struct se_session *sess = se_cmd->se_sess; - assert_spin_locked(&sess->sess_cmd_lock); - WARN_ON_ONCE(!irqs_disabled()); /* * If command already reached CMD_T_COMPLETE state within * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown, diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 74b646f165d4..2c3ddb3a4124 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop, __acquires(&cmd->t_state_lock) { - assert_spin_locked(&cmd->t_state_lock); - WARN_ON_ONCE(!irqs_disabled()); - if (fabric_stop) cmd->transport_state |= CMD_T_FABRIC_STOP; @@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status) { int ret; - assert_spin_locked(&cmd->t_state_lock); - WARN_ON_ONCE(!irqs_disabled()); - if (!(cmd->transport_state & CMD_T_ABORTED)) return 0; /*
There are a few functions which check for if the lock is held (spin_lock_assert()) and the interrupts are disabled (irqs_disabled()). From looking at the code, each function is static, the caller is near by and does spin_lock_irq|safe(). As Linus puts it: |It's not like this is some function that is exported to random users, |and we should check that the calling convention is right. | |This looks like "it may have been useful during coding to document |things, but it's not useful long-term". Remove those checks. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/target/target_core_tmr.c | 2 -- drivers/target/target_core_transport.c | 6 ------ 2 files changed, 8 deletions(-)