Re: uio regression in 4.18-rc1 due to "uio: Prevent device destruction while fds are open"
Hi Mike, Thanks for reporting this problem with my patch. To be honest I'm possibly out of my depth here. I gather doing things like sleeping and memory allocation are a big no-no when under spin_lock and I take your point that it is not realistic to expect all the callouts from the various uio_fops to know or respect that they might be under a spin_lock. I'd take your advise on whether altering the type of locking used around accesses to the 'info' member of struct uio_device might be a path forward here or is likely to lead to similar or worse issues. For example could we change the spin_lock to something more heavyweight like a mutex? My idea of completing Brian Russell's work by introducing this locking construct is also possibly the wrong approach. It seems nice that we can make the uio core cope with the disappearance of the uio_info memory that the info member points to, but I'm beginning to think that we should consider a more fundamental change to allow the uio core to control the life cycle of that uio_info structure. This spin_lock idea seemed like a clean way of avoiding that, but perhaps we just need to lance that boil in a more wide-ranging fashion and accept the consequences such as breaking out-of-tree drivers. Such a wide ranging change would be better done out of the release period and reverting this change would be the best response to this regression for 4.18. If anyone else wants to weigh in here please do so. Thanks, Hamish M. On 06/21/2018 04:06 AM, Mike Christie wrote: > Hi Hamish, > > I am hitting a regression with your patch: > > commit a93e7b331568227500186a465fee3c2cb5dffd1f > Author: Hamish Martin > Date: Mon May 14 13:32:23 2018 +1200 > > uio: Prevent device destruction while fds are open > > The problem is the addition of spin_lock_irqsave in uio_write. This > leads to hitting uio_write -> copy_from_user -> _copy_from_user -> > might_fault and the logs filling up with sleeping warnings. > > I also noticed some uio drivers allocate memory, sleep, grab mutexes > from callouts like open() and release and uio is now doing > spin_lock_irqsave while calling them. > > Note target_core_user's irqcontrol looks buggy and was just doing more > work than it should in that callout. I can fix that driver.
Re: uio regression in 4.18-rc1 due to "uio: Prevent device destruction while fds are open"
Hi Mike, Thanks for reporting this problem with my patch. To be honest I'm possibly out of my depth here. I gather doing things like sleeping and memory allocation are a big no-no when under spin_lock and I take your point that it is not realistic to expect all the callouts from the various uio_fops to know or respect that they might be under a spin_lock. I'd take your advise on whether altering the type of locking used around accesses to the 'info' member of struct uio_device might be a path forward here or is likely to lead to similar or worse issues. For example could we change the spin_lock to something more heavyweight like a mutex? My idea of completing Brian Russell's work by introducing this locking construct is also possibly the wrong approach. It seems nice that we can make the uio core cope with the disappearance of the uio_info memory that the info member points to, but I'm beginning to think that we should consider a more fundamental change to allow the uio core to control the life cycle of that uio_info structure. This spin_lock idea seemed like a clean way of avoiding that, but perhaps we just need to lance that boil in a more wide-ranging fashion and accept the consequences such as breaking out-of-tree drivers. Such a wide ranging change would be better done out of the release period and reverting this change would be the best response to this regression for 4.18. If anyone else wants to weigh in here please do so. Thanks, Hamish M. On 06/21/2018 04:06 AM, Mike Christie wrote: > Hi Hamish, > > I am hitting a regression with your patch: > > commit a93e7b331568227500186a465fee3c2cb5dffd1f > Author: Hamish Martin > Date: Mon May 14 13:32:23 2018 +1200 > > uio: Prevent device destruction while fds are open > > The problem is the addition of spin_lock_irqsave in uio_write. This > leads to hitting uio_write -> copy_from_user -> _copy_from_user -> > might_fault and the logs filling up with sleeping warnings. > > I also noticed some uio drivers allocate memory, sleep, grab mutexes > from callouts like open() and release and uio is now doing > spin_lock_irqsave while calling them. > > Note target_core_user's irqcontrol looks buggy and was just doing more > work than it should in that callout. I can fix that driver.
uio regression in 4.18-rc1 due to "uio: Prevent device destruction while fds are open"
Hi Hamish, I am hitting a regression with your patch: commit a93e7b331568227500186a465fee3c2cb5dffd1f Author: Hamish Martin Date: Mon May 14 13:32:23 2018 +1200 uio: Prevent device destruction while fds are open The problem is the addition of spin_lock_irqsave in uio_write. This leads to hitting uio_write -> copy_from_user -> _copy_from_user -> might_fault and the logs filling up with sleeping warnings. I also noticed some uio drivers allocate memory, sleep, grab mutexes from callouts like open() and release and uio is now doing spin_lock_irqsave while calling them. Note target_core_user's irqcontrol looks buggy and was just doing more work than it should in that callout. I can fix that driver.
uio regression in 4.18-rc1 due to "uio: Prevent device destruction while fds are open"
Hi Hamish, I am hitting a regression with your patch: commit a93e7b331568227500186a465fee3c2cb5dffd1f Author: Hamish Martin Date: Mon May 14 13:32:23 2018 +1200 uio: Prevent device destruction while fds are open The problem is the addition of spin_lock_irqsave in uio_write. This leads to hitting uio_write -> copy_from_user -> _copy_from_user -> might_fault and the logs filling up with sleeping warnings. I also noticed some uio drivers allocate memory, sleep, grab mutexes from callouts like open() and release and uio is now doing spin_lock_irqsave while calling them. Note target_core_user's irqcontrol looks buggy and was just doing more work than it should in that callout. I can fix that driver.
