Re: please fix FUSION (Was: [v3.13][v3.14][Regression]kthread:makekthread_create()killable)
Oleg Nesterov wrote: Tetsuo, what do you think? I don't like blocking SIGKILL while doing operations that depend on memory allocation by other processes. If the OOM killer is triggered and it chose the process blocking SIGKILL in mptsas_init() (I know it unlikely happens), it generates the OOM killer deadlock. My preference is to fix kthread_create() to handle SIGKILL gracefully. kthread_create() did not return upon SIGKILL before commit 786235ee. Since commit 786235ee, there is imbalance that kmalloc(GFP_KERNEL) in kthread_create_on_node() ignores SIGKILL unless TIF_MEMDIE is set but wait_for_completion_killable() in kthread_create_on_node() does not ignore SIGKILL even if TIF_MEMDIE is not set. Many kernel operations (e.g. mutex_lock() wait_event() wait_for_completion()) ignore SIGKILL and the current users depend on SIGKILL being ignored. Thus, commit 786235ee sounds like a kernel API breakage. -- From 731f1f6dec7efaa132f751c432079b9b1fdb78d2 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp Date: Sat, 22 Mar 2014 15:16:50 +0900 Subject: [PATCH] kthread: Handle SIGKILL gracefully in kthread_create(). Commit 786235ee kthread: make kthread_create() killable changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures because systemd-udevd receives SIGKILL due to timeout while loading SCSI controller drivers using finit_module() [1]. Therefore, this patch changes kthread_create() from wait forever in killable state to wait for 10 seconds in unkillable state, check for the OOM killer every second. This patch also changes the return value of timeout case from -ENOMEM to -EINTR because -ENOMEM could make sense for only TIF_MEMDIE case. [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705 Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp --- kernel/kthread.c | 37 + 1 files changed, 21 insertions(+), 16 deletions(-) diff --git a/kernel/kthread.c b/kernel/kthread.c index b5ae3ee..6a25a9f 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -269,6 +269,7 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data), const char namefmt[], ...) { + int i = 0; DECLARE_COMPLETION_ONSTACK(done); struct task_struct *task; struct kthread_create_info *create = kmalloc(sizeof(*create), @@ -287,24 +288,28 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data), wake_up_process(kthreadd_task); /* -* Wait for completion in killable state, for I might be chosen by -* the OOM killer while kthreadd is trying to allocate memory for -* new kernel thread. +* Wait for completion with 10 seconds timeout. Unless the kthreadd is +* blocked for direct memory reclaim path, the kthreadd will be able to +* complete the request within 10 seconds. Also, check every second if +* I was chosen by the OOM killer in order to avoid the OOM killer +* deadlock. */ - if (unlikely(wait_for_completion_killable(done))) { - /* -* If I was SIGKILLed before kthreadd (or new kernel thread) -* calls complete(), leave the cleanup of this structure to -* that thread. -*/ - if (xchg(create-done, NULL)) - return ERR_PTR(-ENOMEM); - /* -* kthreadd (or new kernel thread) will call complete() -* shortly. -*/ - wait_for_completion(done); + do { + if (likely(wait_for_completion_timeout(done, HZ))) + goto ready; + } while (i++ 10 !test_thread_flag(TIF_MEMDIE)); + /* +* The kthreadd was unable to complete the request within 10 seconds +* (or I was chosen by the OOM killer). Thus, give up and leave the +* cleanup of this structure to the kthreadd (or new kernel thread). +*/ + if (xchg(create-done, NULL)) { + WARN(1, Gave up waiting for kthreadd.\n); + return ERR_PTR(-EINTR); } + /* kthreadd (or new kernel thread) will call complete() shortly. */ + wait_for_completion(done); +ready: task = create-result; if (!IS_ERR(task)) { static const struct sched_param param = { .sched_priority = 0 }; -- 1.7.1 -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/5] userspace PI passthrough via AIO/DIO
On Fri, Mar 21, 2014 at 07:32:16PM -0700, Darrick J. Wong wrote: On Fri, Mar 21, 2014 at 05:29:09PM -0700, Zach Brown wrote: On Fri, Mar 21, 2014 at 03:54:37PM -0700, Darrick J. Wong wrote: On Fri, Mar 21, 2014 at 05:44:10PM -0400, Benjamin LaHaise wrote: I'm inclined to agree with Zach on this item. Ultimately, we need an extensible data structure that can be grown without completely revising the ABI as new parameters are added. We need something that is either TLV based, or an extensible array. Ok. Let's define IOCB_FLAG_EXTENSIONS as an iocb.aio_flags flag to indicate that this struct iocb has extensions attached to it. Then, iocb.aio_reserved2 becomes a pointer to an array of extension descriptors, and iocb.aio_reqprio becomes a u16 that tells us the array length. The libaio.h equivalents are iocb.u.c.flags, iocb.u.c.__pad3, and iocb.aio_reqprio, respectively. Next, let's define a conceptual structure for aio extensions: struct iocb_extension { void *ie_buf; unsigned int ie_buflen; unsigned int ie_type; unsigned int ie_flags; }; The actual definitions can be defined in a similar fashion to the other aio structures so that the structures are padded to the same layout regardless of bitness. As mentioned above, iocb.aio_reserved2 points to an array of these. I'm firmly in the camp that doesn't want to go down this abstract road. We had this conversation with Kent when he wanted to do something very similar. Could you point me to this discussion? I'd like to read it. Is it [RFC, PATCH] Extensible AIO interface? http://lkml.iu.edu//hypermail/linux/kernel/1210.0/00651.html Regrettably that discussion happened right during that period where I was pleasantly AWOL from work for a few months. :) Will read ... tomorrow. What happens if there are duplicate ie_types? Is that universally prohibited, validity left up to the types that are duplicated? Yes. What if the len is not the right size? Who checks that? The extension driver, presumably. What if the extension (they're arguments, but one thing at a time) is writable and the buf pointers overlap or is unaligned? Is that cool, who checks it? Each extension driver has to check the alignment. I don't know what to do about buffer pointer overlap; if you want to shoot yourself in the foot that's fine with me. Who defines the acceptable set? (This was an I don't know, for anyone who cares.) Can drivers make up their own weird types? How do you mean? As far as whatever's in the ie_buf, I think that depends on the extension. How does strace print all this? How does the security module universe declare policies that can forbid or allow these things? I don't know. Personally, I think this level of dynamism is not worth the complexity. Can we instead just have a nice easy struct with fixed members that only grows? struct some_more_args { u64 has; /* = HAS_PI_VEC; */ u64 pi_vec_ptr; u64 pi_vec_nr_segs; }; struct some_more_args { u64 has; /* = HAS_PI_VEC | HAS_MAGIC_THING */ u64 pi_vec_ptr; u64 pi_vec_nr_segs; u64 magic_thing; }; If it only grows and has bits indicating presence then I think we're good. You only fetch the space for the bits that are indicated. You can return errors for bits you don't recognize. You could perhaps offer some way to announce the bits you recognize. shrug I was gonna just -EINVAL for types we don't recognize, or which don't apply in this scenario. I'll admit, though, that I don't really like having to fetch the 'has' bits first to find out how large the rest of the struct is. Maybe that's not worth worrying about. I'm not worrying about having to pluck 'has' out of the structure, but needing a function to tell me how big of a buffer I need for a given pile of flags seems ... icky. But maybe the ease of modifying strace and security auditors would make it worth it? How about explicitly specifying the structure size in struct some_more_args, and checking that against whatever we find in .has? Hm. I still think that's too clever for my brain to keep together for long. I'm also nervous that we could be creating this monster of a structure wherein some user wants to tack the first and last hints ever created onto an IO, so now we have to lug this huge structure around that has space for hints that we're not going to use, and most of which is zeroes. I think it would be easy to add one of these interfaces to the regular {read,write}{,v} calls too. --D Thoughts? Am I out to lunch here? I don't have a problem adopting your design, aside from the complications of figuring out how big struct some_more_args really is. Question: Do we want to allow ie_buf to be struct iovec[]? Can we leave that to the extension designer to
[Bug 71941] LSI SAS2116 does not detect disks
https://bugzilla.kernel.org/show_bug.cgi?id=71941 kernel-bugzilla.20.drksha...@spamgourmet.com changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |INVALID --- Comment #1 from kernel-bugzilla.20.drksha...@spamgourmet.com --- The issue turned out to be hardware: Some combination of flashing the firmware and BIOS caused all disks to register. I did get the 18.00.00.00 driver to compile under 3.13.6, however it gave me its own problems: as I attempted to dd if=$f of=$f bs=65536 across every disk in the system simultaneously, the system would, after 20-30 minutes, lock solid. (This process proceeded fine if of=/dev/null; I'm doing this to find disks that I know have sector errors.) I have no dmesg output. Further, modprobe -r was _very_ slow on this driver, eventually printing to dmesg a trace including warn_slowpath_fmt. Attached is a basic patch to get the new LSI driver to compile under 3.13.6, but no guarantees as to the quality of anything. -- You are receiving this mail because: You are watching the assignee of the bug. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Bug 71941] LSI SAS2116 does not detect disks
https://bugzilla.kernel.org/show_bug.cgi?id=71941 --- Comment #2 from kernel-bugzilla.20.drksha...@spamgourmet.com --- Created attachment 130291 -- https://bugzilla.kernel.org/attachment.cgi?id=130291action=edit Patch to LSI 18.00.00.00 driver to compile on 3.13.6 -- You are receiving this mail because: You are watching the assignee of the bug. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RESEND][PATCH 0/2] Cleanup asm/scatterlist.h
I haven't gotten many responses so I'm going to try sending again (this time with a cover letter which I probably should have done in the first place...) ARCH_HAS_SG_CHAIN is currently defined as needed in asm/scatterlist.h. It was suggested[1] that this should probably be a proper Kconfig. At the same time, we can clean up most of the asm/scatterlist.h files to reduce redundancy. This makes parisc the only architecture that still has an asm/scatterlist.h file due to the #define sg_virt_addr (which could probably be cleaned up further still) Andrew, once we get a few more Acked-bys can we take this through your tree as suggested by Will? Thanks, Laura [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/240435.html Laura Abbott (2): lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig Cleanup useless architecture versions of scatterlist.h arch/alpha/include/asm/Kbuild | 1 + arch/alpha/include/asm/scatterlist.h | 6 -- arch/arm/Kconfig | 1 + arch/arm/include/asm/Kbuild | 1 + arch/arm/include/asm/scatterlist.h| 12 arch/arm64/Kconfig| 1 + arch/cris/include/asm/Kbuild | 1 + arch/cris/include/asm/scatterlist.h | 6 -- arch/frv/include/asm/Kbuild | 1 + arch/frv/include/asm/scatterlist.h| 6 -- arch/ia64/Kconfig | 1 + arch/ia64/include/asm/Kbuild | 1 + arch/ia64/include/asm/scatterlist.h | 7 --- arch/m32r/include/asm/Kbuild | 1 + arch/m32r/include/asm/scatterlist.h | 6 -- arch/microblaze/include/asm/Kbuild| 1 + arch/microblaze/include/asm/scatterlist.h | 1 - arch/mn10300/include/asm/Kbuild | 1 + arch/mn10300/include/asm/scatterlist.h| 16 arch/powerpc/Kconfig | 1 + arch/powerpc/include/asm/Kbuild | 1 + arch/powerpc/include/asm/scatterlist.h| 17 - arch/s390/Kconfig | 1 + arch/s390/include/asm/Kbuild | 1 + arch/s390/include/asm/scatterlist.h | 3 --- arch/score/include/asm/Kbuild | 2 +- arch/score/include/asm/scatterlist.h | 6 -- arch/sparc/Kconfig| 1 + arch/sparc/include/asm/Kbuild | 1 + arch/sparc/include/asm/scatterlist.h | 8 arch/x86/Kconfig | 1 + arch/x86/include/asm/Kbuild | 1 + arch/x86/include/asm/scatterlist.h| 8 include/linux/scatterlist.h | 2 +- include/scsi/scsi.h | 2 +- lib/Kconfig | 7 +++ lib/scatterlist.c | 4 ++-- 37 files changed, 31 insertions(+), 107 deletions(-) delete mode 100644 arch/alpha/include/asm/scatterlist.h delete mode 100644 arch/arm/include/asm/scatterlist.h delete mode 100644 arch/cris/include/asm/scatterlist.h delete mode 100644 arch/frv/include/asm/scatterlist.h delete mode 100644 arch/ia64/include/asm/scatterlist.h delete mode 100644 arch/m32r/include/asm/scatterlist.h delete mode 100644 arch/microblaze/include/asm/scatterlist.h delete mode 100644 arch/mn10300/include/asm/scatterlist.h delete mode 100644 arch/powerpc/include/asm/scatterlist.h delete mode 100644 arch/s390/include/asm/scatterlist.h delete mode 100644 arch/score/include/asm/scatterlist.h delete mode 100644 arch/sparc/include/asm/scatterlist.h delete mode 100644 arch/x86/include/asm/scatterlist.h -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
Rather than have architectures #define ARCH_HAS_SG_CHAIN in an architecture specific scatterlist.h, make it a proper Kconfig option and use that instead. At same time, remove the header files are are now mostly useless and just include asm-generic/scatterlist.h. Cc: Russell King li...@arm.linux.org.uk Cc: Tony Luck tony.l...@intel.com Cc: Fenghua Yu fenghua...@intel.com Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Paul Mackerras pau...@samba.org Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Fenghua Yu fenghua...@intel.com Cc: Tony Luck tony.l...@intel.com Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Paul Mackerras pau...@samba.org Cc: Martin Schwidefsky schwidef...@de.ibm.com Cc: Heiko Carstens heiko.carst...@de.ibm.com Cc: Andrew Morton a...@linux-foundation.org Signed-off-by: Laura Abbott lau...@codeaurora.org --- arch/arm/Kconfig | 1 + arch/arm/include/asm/Kbuild| 1 + arch/arm/include/asm/scatterlist.h | 12 arch/arm64/Kconfig | 1 + arch/ia64/Kconfig | 1 + arch/ia64/include/asm/Kbuild | 1 + arch/ia64/include/asm/scatterlist.h| 7 --- arch/powerpc/Kconfig | 1 + arch/powerpc/include/asm/Kbuild| 1 + arch/powerpc/include/asm/scatterlist.h | 17 - arch/s390/Kconfig | 1 + arch/s390/include/asm/Kbuild | 1 + arch/s390/include/asm/scatterlist.h| 3 --- arch/sparc/Kconfig | 1 + arch/sparc/include/asm/Kbuild | 1 + arch/sparc/include/asm/scatterlist.h | 8 arch/x86/Kconfig | 1 + arch/x86/include/asm/Kbuild| 1 + arch/x86/include/asm/scatterlist.h | 8 include/linux/scatterlist.h| 2 +- include/scsi/scsi.h| 2 +- lib/Kconfig| 7 +++ lib/scatterlist.c | 4 ++-- 23 files changed, 24 insertions(+), 59 deletions(-) delete mode 100644 arch/arm/include/asm/scatterlist.h delete mode 100644 arch/ia64/include/asm/scatterlist.h delete mode 100644 arch/powerpc/include/asm/scatterlist.h delete mode 100644 arch/s390/include/asm/scatterlist.h delete mode 100644 arch/sparc/include/asm/scatterlist.h delete mode 100644 arch/x86/include/asm/scatterlist.h diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 1594945..8122294 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -82,6 +82,7 @@ config ARM http://www.arm.linux.org.uk/. config ARM_HAS_SG_CHAIN + select ARCH_HAS_SG_CHAIN bool config NEED_SG_DMA_LENGTH diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild index 3278afe..2357ed6 100644 --- a/arch/arm/include/asm/Kbuild +++ b/arch/arm/include/asm/Kbuild @@ -18,6 +18,7 @@ generic-y += param.h generic-y += parport.h generic-y += poll.h generic-y += resource.h +generic-y += scatterlist.h generic-y += sections.h generic-y += segment.h generic-y += sembuf.h diff --git a/arch/arm/include/asm/scatterlist.h b/arch/arm/include/asm/scatterlist.h deleted file mode 100644 index cefdb8f..000 --- a/arch/arm/include/asm/scatterlist.h +++ /dev/null @@ -1,12 +0,0 @@ -#ifndef _ASMARM_SCATTERLIST_H -#define _ASMARM_SCATTERLIST_H - -#ifdef CONFIG_ARM_HAS_SG_CHAIN -#define ARCH_HAS_SG_CHAIN -#endif - -#include asm/memory.h -#include asm/types.h -#include asm-generic/scatterlist.h - -#endif /* _ASMARM_SCATTERLIST_H */ diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 27bbcfc..f2f95f4 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -2,6 +2,7 @@ config ARM64 def_bool y select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE select ARCH_USE_CMPXCHG_LOCKREF + select ARCH_HAS_SG_CHAIN select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST select ARCH_WANT_OPTIONAL_GPIOLIB select ARCH_WANT_COMPAT_IPC_PARSE_VERSION diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig index 0c8e553..13e2e8b 100644 --- a/arch/ia64/Kconfig +++ b/arch/ia64/Kconfig @@ -44,6 +44,7 @@ config IA64 select HAVE_MOD_ARCH_SPECIFIC select MODULES_USE_ELF_RELA select ARCH_USE_CMPXCHG_LOCKREF + select ARCH_HAS_SG_CHAIN default y help The Itanium Processor Family is Intel's 64-bit successor to diff --git a/arch/ia64/include/asm/Kbuild b/arch/ia64/include/asm/Kbuild index 283a831..3906865 100644 --- a/arch/ia64/include/asm/Kbuild +++ b/arch/ia64/include/asm/Kbuild @@ -2,6 +2,7 @@ generic-y += clkdev.h generic-y += exec.h generic-y += kvm_para.h +generic-y += scatterlist.h generic-y += trace_clock.h generic-y += preempt.h generic-y += vtime.h diff --git a/arch/ia64/include/asm/scatterlist.h b/arch/ia64/include/asm/scatterlist.h deleted file mode 100644 index 08fd93b..000 ---
Re: please fix FUSION (Was: [v3.13][v3.14][Regression]kthread:makekthread_create()killable)
On 03/22, Tetsuo Handa wrote: Oleg Nesterov wrote: Tetsuo, what do you think? I don't like blocking SIGKILL while doing operations that depend on memory allocation by other processes. If the OOM killer is triggered and it chose the process blocking SIGKILL in mptsas_init() (I know it unlikely happens), it generates the OOM killer deadlock. It seems that we do not understand each other. I do not like this hack too. And it is even wrong, you can't really block SIGKILL. And it is unnecessary in a sense that (I think) it is fine that module_init() reacts to SIGKILL and aborts, just the fact it crashes the kernel in the error paths is not fine. The driver should be fixed anyway. As for timeout, either userspace/systemd should be changed to not send SIGKILL after 30 secs, or (better) the driver should be changed to not waste 30 secs. The hack I sent can only serve as a short term solution, it should be reverted once we have something better. And, otoh, this hack only affects the problematic driver which should be fixed in any case. Do you see my point? I can be wrong, but I think that you constantly misunderstand the intent. My preference is to fix kthread_create() to handle SIGKILL gracefully. And now I do not understand you too. I do not understand why should we fix kthread_create(). Many kernel operations (e.g. mutex_lock() wait_event() wait_for_completion()) ignore SIGKILL and the current users depend on SIGKILL being ignored. Thus, commit 786235ee sounds like a kernel API breakage. Personally I do not really think so, but OK. In this case lets revert 786235ee. Commit 786235ee kthread: make kthread_create() killable changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures because systemd-udevd receives SIGKILL due to timeout while loading SCSI controller drivers using finit_module() [1]. And I still think that 786235ee simply uncovered the problems in this driver. Perhaps we should change kthread_create() for some reason, but (imho) not because we need to help the buggy code. Therefore, this patch changes kthread_create() from wait forever in killable state to wait for 10 seconds in unkillable state, check for the OOM killer every second. Personally I dislike this change. In fact I think it is ugly. But this is only my opinion. If you convince someone to take this patch I won't argue. This patch also changes the return value of timeout case from -ENOMEM to -EINTR because -ENOMEM could make sense for only TIF_MEMDIE case. [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705 Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp --- kernel/kthread.c | 37 + 1 files changed, 21 insertions(+), 16 deletions(-) diff --git a/kernel/kthread.c b/kernel/kthread.c index b5ae3ee..6a25a9f 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -269,6 +269,7 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data), const char namefmt[], ...) { + int i = 0; DECLARE_COMPLETION_ONSTACK(done); struct task_struct *task; struct kthread_create_info *create = kmalloc(sizeof(*create), @@ -287,24 +288,28 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data), wake_up_process(kthreadd_task); /* - * Wait for completion in killable state, for I might be chosen by - * the OOM killer while kthreadd is trying to allocate memory for - * new kernel thread. + * Wait for completion with 10 seconds timeout. Unless the kthreadd is + * blocked for direct memory reclaim path, the kthreadd will be able to + * complete the request within 10 seconds. Also, check every second if + * I was chosen by the OOM killer in order to avoid the OOM killer + * deadlock. */ - if (unlikely(wait_for_completion_killable(done))) { - /* - * If I was SIGKILLed before kthreadd (or new kernel thread) - * calls complete(), leave the cleanup of this structure to - * that thread. - */ - if (xchg(create-done, NULL)) - return ERR_PTR(-ENOMEM); - /* - * kthreadd (or new kernel thread) will call complete() - * shortly. - */ - wait_for_completion(done); + do { + if (likely(wait_for_completion_timeout(done, HZ))) + goto ready; + } while (i++ 10 !test_thread_flag(TIF_MEMDIE)); + /* + * The kthreadd was unable to complete the request within 10 seconds + * (or I was chosen by the OOM killer). Thus, give up and leave the + * cleanup of this structure to the kthreadd (or new kernel thread). + */ + if (xchg(create-done, NULL)) { + WARN(1, Gave up
Re: [RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
On Sat, 22 Mar 2014, Laura Abbott wrote: Rather than have architectures #define ARCH_HAS_SG_CHAIN in an architecture specific scatterlist.h, make it a proper Kconfig option and use that instead. At same time, remove the header files are are now mostly useless and just include asm-generic/scatterlist.h. Cc: Russell King li...@arm.linux.org.uk Cc: Tony Luck tony.l...@intel.com Cc: Fenghua Yu fenghua...@intel.com Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Paul Mackerras pau...@samba.org Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Fenghua Yu fenghua...@intel.com Cc: Tony Luck tony.l...@intel.com Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Paul Mackerras pau...@samba.org Cc: Martin Schwidefsky schwidef...@de.ibm.com Cc: Heiko Carstens heiko.carst...@de.ibm.com Cc: Andrew Morton a...@linux-foundation.org Signed-off-by: Laura Abbott lau...@codeaurora.org For the x86 part: Acked-by: Thomas Gleixner t...@linutronix.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: please fix FUSION (Was: [v3.13][v3.14][Regression]kthread:makekthread_create()killable)
On Sat, 2014-03-22 at 20:25 +0100, Oleg Nesterov wrote: On 03/22, Tetsuo Handa wrote: Oleg Nesterov wrote: Tetsuo, what do you think? I don't like blocking SIGKILL while doing operations that depend on memory allocation by other processes. If the OOM killer is triggered and it chose the process blocking SIGKILL in mptsas_init() (I know it unlikely happens), it generates the OOM killer deadlock. It seems that we do not understand each other. I do not like this hack too. And it is even wrong, you can't really block SIGKILL. And it is unnecessary in a sense that (I think) it is fine that module_init() reacts to SIGKILL and aborts, just the fact it crashes the kernel in the error paths is not fine. The driver should be fixed anyway. As for timeout, either userspace/systemd should be changed to not send SIGKILL after 30 secs, or (better) the driver should be changed to not waste 30 secs. The hack I sent can only serve as a short term solution, it should be reverted once we have something better. And, otoh, this hack only affects the problematic driver which should be fixed in any case. Do you see my point? I can be wrong, but I think that you constantly misunderstand the intent. My preference is to fix kthread_create() to handle SIGKILL gracefully. And now I do not understand you too. I do not understand why should we fix kthread_create(). Many kernel operations (e.g. mutex_lock() wait_event() wait_for_completion()) ignore SIGKILL and the current users depend on SIGKILL being ignored. Thus, commit 786235ee sounds like a kernel API breakage. Personally I do not really think so, but OK. In this case lets revert 786235ee. Commit 786235ee kthread: make kthread_create() killable changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures because systemd-udevd receives SIGKILL due to timeout while loading SCSI controller drivers using finit_module() [1]. And I still think that 786235ee simply uncovered the problems in this driver. Perhaps we should change kthread_create() for some reason, but (imho) not because we need to help the buggy code. Therefore, this patch changes kthread_create() from wait forever in killable state to wait for 10 seconds in unkillable state, check for the OOM killer every second. Personally I dislike this change. In fact I think it is ugly. But this is only my opinion. If you convince someone to take this patch I won't argue. OK, the fix from the SCSI point of view is to make the mpt teardown path actually work. The whole thing looks to be a complete mess because there's another place where it will do the wrong thing in mptscsih_remove(). You always have to call mpt_detach() otherwise the device doesn't get removed from the lists. In theory this patch fixes both bugs in the driver. James --- diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c index 727819c..282d39a 100644 --- a/drivers/message/fusion/mptscsih.c +++ b/drivers/message/fusion/mptscsih.c @@ -1176,10 +1176,14 @@ mptscsih_remove(struct pci_dev *pdev) MPT_SCSI_HOST *hd; int sz1; + if (!host) + /* not brought up far enough to do scsi_host_attach() */ + goto out; + scsi_remove_host(host); if((hd = shost_priv(host)) == NULL) - return; + goto out; mptscsih_shutdown(pdev); @@ -1203,6 +1207,7 @@ mptscsih_remove(struct pci_dev *pdev) scsi_host_put(host); + out: mpt_detach(pdev); } -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: please fix FUSION (Was: [v3.13][v3.14][Regression]kthread:makekthread_create()killable)
On Sat, 22 Mar 2014, Oleg Nesterov wrote: On 03/22, Tetsuo Handa wrote: Many kernel operations (e.g. mutex_lock() wait_event() wait_for_completion()) ignore SIGKILL and the current users depend on SIGKILL being ignored. Thus, commit 786235ee sounds like a kernel API breakage. Personally I do not really think so, but OK. In this case lets revert 786235ee. We have explicitely: mutex_lock, mutex_lock_killable and mutex_lock_interruptible. Ditto for wait_event and wait_for_completion. So the existance of the uninterruptible versions does not make an argument for the kthread_create() case. Commit 786235ee kthread: make kthread_create() killable changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures because systemd-udevd receives SIGKILL due to timeout while loading SCSI controller drivers using finit_module() [1]. And I still think that 786235ee simply uncovered the problems in this driver. Perhaps we should change kthread_create() for some reason, but (imho) not because we need to help the buggy code. Right. Therefore, this patch changes kthread_create() from wait forever in killable state to wait for 10 seconds in unkillable state, check for the OOM killer every second. Personally I dislike this change. In fact I think it is ugly. But this is only my opinion. It's not only ugly, it's activly wrong. It's as wrong as 786235ee itself. And 786235ee needs to be reverted and the revert must go into 3.13.stable as well. I'll send a revert request in separate mail. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
On Sat, 2014-03-22 at 11:13 -0700, Laura Abbott wrote: Rather than have architectures #define ARCH_HAS_SG_CHAIN in an architecture specific scatterlist.h, make it a proper Kconfig option and use that instead. At same time, remove the header files are are now mostly useless and just include asm-generic/scatterlist.h. Well, the transformation looks fine. Perhaps part of the reason for the lack of response is that there's no compelling reason in the change log above for doing this. The usual reason for eliminating ARCH_HAS is that it's hiding something that would be better expressed a different way (that's actually intuitive to grep) or that it's expressing something that should be configurable. Neither of these reasons apply in this case, because SG_CHAIN definitely is a property of the architecture not the config space and it's not really hiding anything. Perhaps now might be the time to ask which are the remaining architectures that cannot do SG chaining and then we can fix them and pull the whole thing out. James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: please fix FUSION (Was: [v3.13][v3.14][Regression]kthread:makekthread_create()killable)
On Sat, 22 Mar 2014, Thomas Gleixner wrote: On Sat, 22 Mar 2014, Oleg Nesterov wrote: Personally I dislike this change. In fact I think it is ugly. But this is only my opinion. It's not only ugly, it's activly wrong. It's as wrong as 786235ee itself. And 786235ee needs to be reverted and the revert must go into 3.13.stable as well. I'll send a revert request in separate mail. Sorry. I misread the combo of both patches. 786235ee is correct. We definitely don't want to revert it. But I still think, that changing this to butt ugly heuristics with an arbitrary chosen timeout is not the proper solution to fix a driver problem and to work around a systemd policy which mandates that modprobe must return in 30 seconds. But then systemd/udev mutters: You migh be able to work around the timeout with udev rules and OPTIONS+=event_timeout=120, but that code was maybe never used or tested, so it might not work correctly. [1] AFAICT from the ubuntu bug system [2] nobody bothered even to try that. And if the udev/systemd event_timeout option is broken it's way better to fix that one instead of hacking random heuristics into the kernel. Thanks, tglx [1] http://lists.freedesktop.org/archives/systemd-devel/2014-March/018007.html [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705 -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
On Sat, 22 Mar 2014, James Bottomley wrote: On Sat, 2014-03-22 at 11:13 -0700, Laura Abbott wrote: Rather than have architectures #define ARCH_HAS_SG_CHAIN in an architecture specific scatterlist.h, make it a proper Kconfig option and use that instead. At same time, remove the header files are are now mostly useless and just include asm-generic/scatterlist.h. Well, the transformation looks fine. Perhaps part of the reason for the lack of response is that there's no compelling reason in the change log above for doing this. The usual reason for eliminating ARCH_HAS is that it's hiding something that would be better expressed a different way (that's actually intuitive to grep) or that it's expressing something that should be configurable. Neither of these reasons apply in this case, because SG_CHAIN definitely is a property of the architecture not the config space and it's not really hiding anything. Getting rid of pointless copied code is definitely a good enough reason and the patch removes quite some of that. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
On Sat, Mar 22, 2014 at 02:31:21PM -0700, James Bottomley wrote: Perhaps now might be the time to ask which are the remaining architectures that cannot do SG chaining and then we can fix them and pull the whole thing out. Not quite. You're making the assumption that we can be sure that all the scatterlist users on an architecture have been converted - that's simply not true on ARM. We have some which have, and some which still have not been audited. The cases that get us here would be old platform DMA code which walks scatterlists handed to it from drivers - stuff like arch/arm/mach-rpc/dma.c (which probably can cope), and drivers/scsi/arm/* (which definitely can't because of their SCSI pointers save/restore handling message.) I know that's one case where SG_CHAIN definitely isn't supported on ARM. So, we had decided not to enable it, but this means that new stuff isn't benefitting from this. I've recently asked arm-soc to enable it for the modern multi-platform builds, because modern stuff really be written with correct SG chaining in mind. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
On Sat, 2014-03-22 at 22:23 +, Russell King - ARM Linux wrote: On Sat, Mar 22, 2014 at 02:31:21PM -0700, James Bottomley wrote: Perhaps now might be the time to ask which are the remaining architectures that cannot do SG chaining and then we can fix them and pull the whole thing out. Not quite. You're making the assumption that we can be sure that all the scatterlist users on an architecture have been converted - that's simply not true on ARM. No I'm not, I said now might be the time to ask which are the remaining architectures that cannot do SG chaining I think it's time to list them so we know what work remains. I know we've got a bunch in parisc (all of our iommu code in driver/parisc - about 5 different ones - are unconverted). However, the conversion is pretty simple; it's mostly replacing sglist++ with sglist=sg_next(sglist) We have some which have, and some which still have not been audited. The cases that get us here would be old platform DMA code which walks scatterlists handed to it from drivers - stuff like arch/arm/mach-rpc/dma.c (which probably can cope), and drivers/scsi/arm/* (which definitely can't because of their SCSI pointers save/restore handling message.) I know that's one case where SG_CHAIN definitely isn't supported on ARM. So, we had decided not to enable it, but this means that new stuff isn't benefitting from this. I've recently asked arm-soc to enable it for the modern multi-platform builds, because modern stuff really be written with correct SG chaining in mind. OK, so lets see what the actual effort is. James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
On Sat, Mar 22, 2014 at 03:37:40PM -0700, James Bottomley wrote: On Sat, 2014-03-22 at 22:23 +, Russell King - ARM Linux wrote: On Sat, Mar 22, 2014 at 02:31:21PM -0700, James Bottomley wrote: Perhaps now might be the time to ask which are the remaining architectures that cannot do SG chaining and then we can fix them and pull the whole thing out. Not quite. You're making the assumption that we can be sure that all the scatterlist users on an architecture have been converted - that's simply not true on ARM. No I'm not, I said now might be the time to ask which are the remaining architectures that cannot do SG chaining And I'm disagreeing with that statement which implies that it's something that is an architecture wide property for any particular architecture. Right now in mainline, if ARM has IOMMU support enabled, then SG_CHAIN support will also be enabled. I've a patch out of tree which I've been using for years which enables SG_CHAIN for a particular SoC (Dove). Otherwise, it doesn't have support for SG_CHAIN. PARISC on the other hand (as you list) has no support to enable SG_CHAIN under any circumstances. Where we're disagreeing is whether this is something that is always-on or always-off for any particular architecture. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
On Sat, 2014-03-22 at 22:52 +, Russell King - ARM Linux wrote: On Sat, Mar 22, 2014 at 03:37:40PM -0700, James Bottomley wrote: On Sat, 2014-03-22 at 22:23 +, Russell King - ARM Linux wrote: On Sat, Mar 22, 2014 at 02:31:21PM -0700, James Bottomley wrote: Perhaps now might be the time to ask which are the remaining architectures that cannot do SG chaining and then we can fix them and pull the whole thing out. Not quite. You're making the assumption that we can be sure that all the scatterlist users on an architecture have been converted - that's simply not true on ARM. No I'm not, I said now might be the time to ask which are the remaining architectures that cannot do SG chaining And I'm disagreeing with that statement which implies that it's something that is an architecture wide property for any particular architecture. Right now in mainline, if ARM has IOMMU support enabled, then SG_CHAIN support will also be enabled. I've a patch out of tree which I've been using for years which enables SG_CHAIN for a particular SoC (Dove). Otherwise, it doesn't have support for SG_CHAIN. PARISC on the other hand (as you list) has no support to enable SG_CHAIN under any circumstances. Where we're disagreeing is whether this is something that is always-on or always-off for any particular architecture. Actually, I don't disagree with that. PA used to share sb_iommu with ia64 (it's the same chipset for the HP versions), but we can't now because ia64 is chained and we're not and there's no way to say chained for this platform but not for these other more legacy ones. If you have a proposal for this, I'd be interested, so I don't have to do an all or nothing conversion, but the config option isn't it because our platform configuration is runtime determined (we usually select every driver and let the actual one be chosen at runtime from the config table). James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: please fix FUSION (Was:[v3.13][v3.14][Regression]kthread:makekthread_create()killable)
Oleg Nesterov wrote: On 03/22, Tetsuo Handa wrote: Oleg Nesterov wrote: Tetsuo, what do you think? I don't like blocking SIGKILL while doing operations that depend on memory allocation by other processes. If the OOM killer is triggered and it chose the process blocking SIGKILL in mptsas_init() (I know it unlikely happens), it generates the OOM killer deadlock. It seems that we do not understand each other. I'm agreeing with you regarding long term solution. I think that I and you do not understand each other regarding which approach should be used for short term solution. I do not like this hack too. And it is even wrong, you can't really block SIGKILL. And it is unnecessary in a sense that (I think) it is fine that module_init() reacts to SIGKILL and aborts, just the fact it crashes the kernel in the error paths is not fine. I expect that kernel code reacts to SIGKILL and aborts, but current code (not only fusion but any code) is not ready for reacting to SIGKILL due to use of uninterruptible versions of lock/wait etc. operators. The driver should be fixed anyway. As for timeout, either userspace/systemd should be changed to not send SIGKILL after 30 secs, or (better) the driver should be changed to not waste 30 secs. I'm not asserting that we should not fix the driver and the userspace. I agree that we should fix the driver to respond SIGKILL properly and fix the userspace not to send SIGKILL on hard coded timeout. The hack I sent can only serve as a short term solution, it should be reverted once we have something better. And, otoh, this hack only affects the problematic driver which should be fixed in any case. Do you see my point? I can be wrong, but I think that you constantly misunderstand the intent. My preference is to fix kthread_create() to handle SIGKILL gracefully. And now I do not understand you too. I do not understand why should we fix kthread_create(). I can see your point. But as for kernel for Ubuntu 14.04 LTS (which the date of kernel freeze comes shortly), fixing kthread_create() is the safer choice, for we haven't proved that the fusion is the only kernel code which is disturbed by commit 786235ee. After we confirmed that there is no more kernel code which is disturbed by commit 786235ee, we can revert the kthread: Handle SIGKILL gracefully in kthread_create(). patch. Many kernel operations (e.g. mutex_lock() wait_event() wait_for_completion()) ignore SIGKILL and the current users depend on SIGKILL being ignored. Thus, commit 786235ee sounds like a kernel API breakage. Personally I do not really think so, but OK. In this case lets revert 786235ee. Commit 786235ee kthread: make kthread_create() killable changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures because systemd-udevd receives SIGKILL due to timeout while loading SCSI controller drivers using finit_module() [1]. And I still think that 786235ee simply uncovered the problems in this driver. Perhaps we should change kthread_create() for some reason, but (imho) not because we need to help the buggy code. I don't mean to help the buggy code. The kthread: Handle SIGKILL gracefully in kthread_create(). patch (or revert commit 786235ee) is short term solution (especially for distributions which the date of kernel freeze is approaching). Therefore, this patch changes kthread_create() from wait forever in killable state to wait for 10 seconds in unkillable state, check for the OOM killer every second. Personally I dislike this change. In fact I think it is ugly. But this is only my opinion. If you convince someone to take this patch I won't argue. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: please fix FUSION (Was:[v3.13][v3.14][Regression]kthread:makekthread_create()killable)
Thomas Gleixner wrote: But then systemd/udev mutters: You migh be able to work around the timeout with udev rules and OPTIONS+=event_timeout=120, but that code was maybe never used or tested, so it might not work correctly. [1] AFAICT from the ubuntu bug system [2] nobody bothered even to try that. And if the udev/systemd event_timeout option is broken it's way better to fix that one instead of hacking random heuristics into the kernel. I haven't tried the event_timeout= option but I think it will not work. The timeout is hard coded as shown below and there will be no chance for taking the event_timeout= option into account. -- systemd-204/src/udev/udevd.c start -- (...snipped...) /* check for hanging events */ udev_list_node_foreach(loop, worker_list) { struct worker *worker = node_to_worker(loop); if (worker-state != WORKER_RUNNING) continue; if ((now(CLOCK_MONOTONIC) - worker-event_start_usec) 30 * 1000 * 1000) { log_error(worker [%u] %s timeout; kill it\n, worker-pid, worker-event ? worker-event-devpath : idle); kill(worker-pid, SIGKILL); worker-state = WORKER_KILLED; /* drop reference taken for state 'running' */ worker_unref(worker); if (worker-event) { log_error(seq %llu '%s' killed\n, udev_device_get_seqnum(worker-event-dev), worker-event-devpath); worker-event-exitcode = -64; event_queue_delete(worker-event, true); worker-event = NULL; } } } (...snipped...) -- systemd-204/src/udev/udevd.c end -- -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 1594945..8122294 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -82,6 +82,7 @@ config ARM http://www.arm.linux.org.uk/. config ARM_HAS_SG_CHAIN + select ARCH_HAS_SG_CHAIN bool Heh, a self-selecting config option... I didn't know that trick ! Ben. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH 1/2] lib/scatterlist: Make ARCH_HAS_SG_CHAIN an actual Kconfig
On Sat, 2014-03-22 at 11:13 -0700, Laura Abbott wrote: Rather than have architectures #define ARCH_HAS_SG_CHAIN in an architecture specific scatterlist.h, make it a proper Kconfig option and use that instead. At same time, remove the header files are are now mostly useless and just include asm-generic/scatterlist.h. Cc: Russell King li...@arm.linux.org.uk Cc: Tony Luck tony.l...@intel.com Cc: Fenghua Yu fenghua...@intel.com For powerpc Acked-by: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Paul Mackerras pau...@samba.org Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Fenghua Yu fenghua...@intel.com Cc: Tony Luck tony.l...@intel.com Cc: Benjamin Herrenschmidt b...@kernel.crashing.org Cc: Paul Mackerras pau...@samba.org Cc: Martin Schwidefsky schwidef...@de.ibm.com Cc: Heiko Carstens heiko.carst...@de.ibm.com Cc: Andrew Morton a...@linux-foundation.org Signed-off-by: Laura Abbott lau...@codeaurora.org -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html