Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()
On 2018-03-23 17:44:46 [+], Bart Van Assche wrote: > On Fri, 2018-03-23 at 18:19 +0100, bige...@linutronix.de wrote: > > __target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the > > caller > > holds lun_tg_pt_gp_lock(). Both functions are static, the callers are > > acquiring the lock before the invocation of the function so the check > > looks superfluous. > > Remove it. > > Does this check cause trouble to anyone or to a specific kernel configuration? Those two do not. > In other words, do we really need to remove these checks? I think that these > checks are useful as documentation to people who read the SCSI target code. > The target code is already hard to follow so I think any documentation, > especially documentation in the form of code that is checked at runtime, is > welcome. so if I remove those two and add a kernel doc comment instead, saying that the caller needs to ensure that "lun->lun_tg_pt_gp_lock" is held then we would remove the obvious runtime check and add a piece of documentation. Would that work? > Thanks, > > Bart. Sebastian
Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())
On 2018-03-23 16:25:25 [+], Bart Van Assche wrote: > On Fri, 2018-03-23 at 16:55 +0100, Sebastian Andrzej Siewior wrote: > > I am going take this into -RT tree for now until we have different > > solution. > > Have you considered to delete the WARN_ON_ONCE(!irqs_disabled()) statement? > I think that check duplicates functionality that already exists in lockdep > since lockdep is already able to detect spinlock use inconsistencies. correct. That is why I suggested to use lockdep_assert_held() instead of this IRQ-check + the spin_lock_assert(). The only downside is that this code (as of now) works with lockdep disabled. On the other hand, lockdep_assert_held() gives you a splat instead of a BUG() statement like spin_lock_assert() does so I clearly promote lockdep here :) > Bart. Sebastian
[PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks
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
---
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
@@ -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;
/*
--
2.16.2
[PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()
__target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the caller
holds lun_tg_pt_gp_lock(). Both functions are static, the callers are
acquiring the lock before the invocation of the function so the check
looks superfluous.
Remove it.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/target/target_core_alua.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/target/target_core_alua.c
b/drivers/target/target_core_alua.c
index e46ca968009c..e5bda674bdbd 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -1843,8 +1843,6 @@ static void __target_attach_tg_pt_gp(struct se_lun *lun,
{
struct se_dev_entry *se_deve;
- assert_spin_locked(&lun->lun_tg_pt_gp_lock);
-
spin_lock(&tg_pt_gp->tg_pt_gp_lock);
lun->lun_tg_pt_gp = tg_pt_gp;
list_add_tail(&lun->lun_tg_pt_gp_link, &tg_pt_gp->tg_pt_gp_lun_list);
@@ -1868,8 +1866,6 @@ void target_attach_tg_pt_gp(struct se_lun *lun,
static void __target_detach_tg_pt_gp(struct se_lun *lun,
struct t10_alua_tg_pt_gp *tg_pt_gp)
{
- assert_spin_locked(&lun->lun_tg_pt_gp_lock);
-
spin_lock(&tg_pt_gp->tg_pt_gp_lock);
list_del_init(&lun->lun_tg_pt_gp_link);
tg_pt_gp->tg_pt_gp_members--;
--
2.16.2
Re: iscsi_trx oops in 4.18.0-rc2 and potential patch for percpu_ida.c
On 2018-06-28 19:53:21 [+], Calciano, Jess wrote:
> So although the problem has already been reported, we're wondering if there
> are any updates on the status of the fix, or if it will be available in an
> upcoming mainline build.
In Linus' tree:
4bb6e96ab808 ("lib/percpu_ida.c: don't do alloc from per-CPU list if there is
none")
> Thanks,
> Jess
Sebastian
Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks
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
Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks
On 2018-03-28 15:05:41 [+], 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
Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks
On 2018-03-28 08:31:38 [-0700], Bart Van Assche wrote: > On 03/28/18 08:14, bige...@linutronix.de wrote: > > On 2018-03-28 15:05:41 [+], 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
Re: [RT PATCH v2] net/xfrm/xfrm_ipcomp: Protect scratch buffer with local_lock
On 2019-08-21 08:44:22 [+0200], Juri Lelli wrote: > > Hi Juri, currently if the mail subject has RFC, we will test it but send > > report > > privately to author only. > > OK. But, my email had "RT" and not "RFC" in the subject (since it is > meant for one of the PREEMPT_RT stable trees [1]). Was the RT tag consider at all at some point? I remember I asked for it and then the bot did not complain again or I don't remember. At the same point my rt-devel tree was added to the trees-to-be-tested. > Best, > > Juri > > 1 - git://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git > v4.19-rt Sebastian
Re: [ptrace, rt] erratic behaviour in PTRACE_SINGLESET on 4.13-rt and later.
On 2018-11-27 15:10:07 [+], Joe Korty wrote: > Hi Clark, Hi Clark, > No I don't, sorry. I am attaching the LAG version, it is a few > dozen lines shorter than the version I first sent out to the > mailing list. Did this make its way to rt-tests? I have some changes on top of it while I was looking for the bug… Sebastian
Re: [PATCH] perf/tracing/cpuhotplug: don't release cred_guard_mutex if not taken
On 2017-06-03 03:39:13 [+], Levin, Alexander (Sasha Levin) wrote:
> From: Alexander Levin
>
> If we failed to acquire task's cred_guard_mutex we shouldn't proceed
> to release it in the error path.
>
> Fixes: a63fbed776c ("perf/tracing/cpuhotplug: Fix locking order")
> Signed-off-by: Alexander Levin
Acked-by: Sebastian Andrzej Siewior
Sebastian
