Re: [PATCH 00/10 v3] iommu/amd: lock splitting & GFP_KERNEL allocation
On Thu, Mar 22, 2018 at 04:22:32PM +0100, Sebastian Andrzej Siewior wrote: > Hi, > > this is the rebase on top of iommu/x86/amd of my last series. It takes > Scotts comments into consideration from my v2. > > It contains lock splitting and GFP_KERNEL allocation of remap-table. > Mostly cleanup. > > The patches were boot tested on an AMD EPYC 7601. Applied, thanks.
[PATCH 00/10 v3] iommu/amd: lock splitting & GFP_KERNEL allocation
Hi, this is the rebase on top of iommu/x86/amd of my last series. It takes Scotts comments into consideration from my v2. It contains lock splitting and GFP_KERNEL allocation of remap-table. Mostly cleanup. The patches were boot tested on an AMD EPYC 7601. Sebastian
Re: [PATCH 00/10 v2] iommu/amd: lock splitting & GFP_KERNEL allocation
On Mon, 2018-03-19 at 13:15 +0100, Sebastian Andrzej Siewior wrote: > On 2018-03-17 16:43:39 [-0500], Scott Wood wrote: > > If that's worth the lock dropping then fine (though why does only > > one > > of the two allocations use GFP_KERNEL?), but it doesn't need to be > > a > > That was a mistake, I planned to keep both as GFP_KERNEL. > > > raw lock if the non-allocating users are separated. Keeping them > > separate will also preserve the WARNs if we somehow end up in an > > atomic > > context with no table (versus relying on atomic sleep debugging > > that > > may or may not be enabled), and make the code easier to understand > > by > > being explicit about which functions can be used from RT-atomic > > context. > > That separated part is okay. We could keep it. However, I am not sure > if > looking at the table irq_lookup_table[devid] without the lock is > okay. > The pointer is assigned without DTE entry/iommu-flush to be > completed. > This does not look "okay". Those callers are getting the devid from an irq_2_irte struct, which was set up in irq_remapping_alloc() after get/alloc_irq_table() is completed. -Scott
Re: [PATCH 00/10 v2] iommu/amd: lock splitting & GFP_KERNEL allocation
On 2018-03-17 16:43:39 [-0500], Scott Wood wrote: > If that's worth the lock dropping then fine (though why does only one > of the two allocations use GFP_KERNEL?), but it doesn't need to be a That was a mistake, I planned to keep both as GFP_KERNEL. > raw lock if the non-allocating users are separated. Keeping them > separate will also preserve the WARNs if we somehow end up in an atomic > context with no table (versus relying on atomic sleep debugging that > may or may not be enabled), and make the code easier to understand by > being explicit about which functions can be used from RT-atomic > context. That separated part is okay. We could keep it. However, I am not sure if looking at the table irq_lookup_table[devid] without the lock is okay. The pointer is assigned without DTE entry/iommu-flush to be completed. This does not look "okay". > -Scott > Sebastian
Re: [PATCH 00/10 v2] iommu/amd: lock splitting & GFP_KERNEL allocation
On Sat, 2018-03-17 at 22:10 +0100, Sebastian Andrzej Siewior wrote: > On 2018-03-17 14:49:54 [-0500], Scott Wood wrote: > > On Fri, 2018-03-16 at 21:18 +0100, Sebastian Andrzej Siewior wrote: > > > The goal here is to make the memory allocation in get_irq_table() > > > not > > > with disabled interrupts and having as little raw_spin_lock as > > > possible > > > while having them if the caller is also holding one (like desc- > > > >lock > > > during IRQ-affinity changes). > > > I reverted one patch one patch in the iommu while rebasing since > > > it > > > make job easier. > > > > If the goal is to have "as little raw_spin_lock as possible" -- and > > presumably also to avoid unnecessary complexity -- wouldn't it be > > better to leave my patch in, and drop patches 4 and 9? > > 9 gives me GFP_KERNEL instead atomic so no. > 4 is needed I think but I could double check on Monday. If that's worth the lock dropping then fine (though why does only one of the two allocations use GFP_KERNEL?), but it doesn't need to be a raw lock if the non-allocating users are separated. Keeping them separate will also preserve the WARNs if we somehow end up in an atomic context with no table (versus relying on atomic sleep debugging that may or may not be enabled), and make the code easier to understand by being explicit about which functions can be used from RT-atomic context. -Scott
Re: [PATCH 00/10 v2] iommu/amd: lock splitting & GFP_KERNEL allocation
On 2018-03-17 14:49:54 [-0500], Scott Wood wrote: > On Fri, 2018-03-16 at 21:18 +0100, Sebastian Andrzej Siewior wrote: > > The goal here is to make the memory allocation in get_irq_table() not > > with disabled interrupts and having as little raw_spin_lock as > > possible > > while having them if the caller is also holding one (like desc->lock > > during IRQ-affinity changes). > > I reverted one patch one patch in the iommu while rebasing since it > > make job easier. > > If the goal is to have "as little raw_spin_lock as possible" -- and > presumably also to avoid unnecessary complexity -- wouldn't it be > better to leave my patch in, and drop patches 4 and 9? 9 gives me GFP_KERNEL instead atomic so no. 4 is needed I think but I could double check on Monday. > -Scott Sebastian
Re: [PATCH 00/10 v2] iommu/amd: lock splitting & GFP_KERNEL allocation
On Fri, 2018-03-16 at 21:18 +0100, Sebastian Andrzej Siewior wrote: > The goal here is to make the memory allocation in get_irq_table() not > with disabled interrupts and having as little raw_spin_lock as > possible > while having them if the caller is also holding one (like desc->lock > during IRQ-affinity changes). > I reverted one patch one patch in the iommu while rebasing since it > make job easier. If the goal is to have "as little raw_spin_lock as possible" -- and presumably also to avoid unnecessary complexity -- wouldn't it be better to leave my patch in, and drop patches 4 and 9? -Scott
[PATCH 00/10 v2] iommu/amd: lock splitting & GFP_KERNEL allocation
The goal here is to make the memory allocation in get_irq_table() not with disabled interrupts and having as little raw_spin_lock as possible while having them if the caller is also holding one (like desc->lock during IRQ-affinity changes). I reverted one patch one patch in the iommu while rebasing since it make job easier. The patches were boot tested on an AMD EPYC 7601. Sebastian
Re: iommu/amd: lock splitting & GFP_KERNEL allocation
Hi Sebastian, On Fri, Feb 23, 2018 at 11:27:26PM +0100, Sebastian Andrzej Siewior wrote: > I have no idea why but suddenly my A10 box complained loudly about > locking and memory allocations within the iommu code under RT. Looking > at the code it has been like this for a longer time so the iommu must > have appeared recently (well there was a bios upgrade due to other > issues so it might have enabled it). > > The goal here is to make the memory allocation in get_irq_table() not > with disabled interrupts and having as little raw_spin_lock as possible > while having them if the caller is also holding one (like desc->lock > during IRQ-affinity changes). > > The patches were boot tested on an AMD EPYC 7601. Thanks for these patches, I really like the cleanups and the improved locking. Please rebase against x86/amd in the iommu branch and address the other comment I have, then I put them into my tree. Thanks, Joerg
iommu/amd: lock splitting & GFP_KERNEL allocation
Hi, I have no idea why but suddenly my A10 box complained loudly about locking and memory allocations within the iommu code under RT. Looking at the code it has been like this for a longer time so the iommu must have appeared recently (well there was a bios upgrade due to other issues so it might have enabled it). The goal here is to make the memory allocation in get_irq_table() not with disabled interrupts and having as little raw_spin_lock as possible while having them if the caller is also holding one (like desc->lock during IRQ-affinity changes). The patches were boot tested on an AMD EPYC 7601. Sebastian
