| Message ID | 1638858768-9971-1-git-send-email-wangzhou1@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "iommu/arm-smmu-v3: Decrease the queue size of evtq and priq" | expand |
On Tue, Dec 07, 2021 at 02:32:48PM +0800, Zhou Wang wrote: > The commit f115f3c0d5d8 ("iommu/arm-smmu-v3: Decrease the queue size of > evtq and priq") decreases evtq and priq, which may lead evtq/priq to be > full with fault events, e.g HiSilicon ZIP/SEC/HPRE have maximum 1024 queues > in one device, every queue could be binded with one process and trigger a > fault event. So let's revert f115f3c0d5d8. > > In fact, if an implementation of SMMU really does not need so long evtq > and priq, value of IDR1_EVTQS and IDR1_PRIQS can be set to proper ones. > > Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) I'd like an Ack from Zhen Lei on this, as the aim of the original patch was to reduce memory consumption. Will
On 2021-12-14 14:48, Will Deacon wrote: > On Tue, Dec 07, 2021 at 02:32:48PM +0800, Zhou Wang wrote: >> The commit f115f3c0d5d8 ("iommu/arm-smmu-v3: Decrease the queue size of >> evtq and priq") decreases evtq and priq, which may lead evtq/priq to be >> full with fault events, e.g HiSilicon ZIP/SEC/HPRE have maximum 1024 queues >> in one device, every queue could be binded with one process and trigger a >> fault event. So let's revert f115f3c0d5d8. >> >> In fact, if an implementation of SMMU really does not need so long evtq >> and priq, value of IDR1_EVTQS and IDR1_PRIQS can be set to proper ones. >> >> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> >> --- >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) > > I'd like an Ack from Zhen Lei on this, as the aim of the original patch > was to reduce memory consumption. I wonder if it's time to start trying to be a bit cleverer than just allocating the smallest/largest possible queues, and try to scale them to some heuristic of the "size" of the system? Certainly a module parameter so that users can manually tune for their system/workload might be a reasonable compromise. I'm also tempted by the idea of trying to dynamically reallocate larger queues if they fill up, but I guess that depends on whether it's already game over if events or PRI requests are lost... Robin.
On 2021/12/14 22:48, Will Deacon wrote: > On Tue, Dec 07, 2021 at 02:32:48PM +0800, Zhou Wang wrote: >> The commit f115f3c0d5d8 ("iommu/arm-smmu-v3: Decrease the queue size of >> evtq and priq") decreases evtq and priq, which may lead evtq/priq to be >> full with fault events, e.g HiSilicon ZIP/SEC/HPRE have maximum 1024 queues >> in one device, every queue could be binded with one process and trigger a >> fault event. So let's revert f115f3c0d5d8. >> >> In fact, if an implementation of SMMU really does not need so long evtq >> and priq, value of IDR1_EVTQS and IDR1_PRIQS can be set to proper ones. >> >> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> >> --- >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) > > I'd like an Ack from Zhen Lei on this, as the aim of the original patch > was to reduce memory consumption. I did it for the purpose of saving memory. At the time, I didn't think it would have accumulated so many events. Now there is such a practical situation. Ensuring functionality stability is more important than saving a little memory. So I have no objection to reverting my patch. Acked-by: Zhen Lei <thunder.leizhen@huawei.com> > > Will > . >
On Tue, 7 Dec 2021 14:32:48 +0800, Zhou Wang wrote: > The commit f115f3c0d5d8 ("iommu/arm-smmu-v3: Decrease the queue size of > evtq and priq") decreases evtq and priq, which may lead evtq/priq to be > full with fault events, e.g HiSilicon ZIP/SEC/HPRE have maximum 1024 queues > in one device, every queue could be binded with one process and trigger a > fault event. So let's revert f115f3c0d5d8. > > In fact, if an implementation of SMMU really does not need so long evtq > and priq, value of IDR1_EVTQS and IDR1_PRIQS can be set to proper ones. > > [...] Applied to will (for-joerg/arm-smmu/updates), thanks! [1/1] Revert "iommu/arm-smmu-v3: Decrease the queue size of evtq and priq" https://git.kernel.org/will/c/477436699e78 Cheers,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index 4cb136f..cd48590 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -184,7 +184,6 @@ #else #define Q_MAX_SZ_SHIFT (PAGE_SHIFT + MAX_ORDER - 1) #endif -#define Q_MIN_SZ_SHIFT (PAGE_SHIFT) /* * Stream table. @@ -374,7 +373,7 @@ /* Event queue */ #define EVTQ_ENT_SZ_SHIFT 5 #define EVTQ_ENT_DWORDS ((1 << EVTQ_ENT_SZ_SHIFT) >> 3) -#define EVTQ_MAX_SZ_SHIFT (Q_MIN_SZ_SHIFT - EVTQ_ENT_SZ_SHIFT) +#define EVTQ_MAX_SZ_SHIFT (Q_MAX_SZ_SHIFT - EVTQ_ENT_SZ_SHIFT) #define EVTQ_0_ID GENMASK_ULL(7, 0) @@ -400,7 +399,7 @@ /* PRI queue */ #define PRIQ_ENT_SZ_SHIFT 4 #define PRIQ_ENT_DWORDS ((1 << PRIQ_ENT_SZ_SHIFT) >> 3) -#define PRIQ_MAX_SZ_SHIFT (Q_MIN_SZ_SHIFT - PRIQ_ENT_SZ_SHIFT) +#define PRIQ_MAX_SZ_SHIFT (Q_MAX_SZ_SHIFT - PRIQ_ENT_SZ_SHIFT) #define PRIQ_0_SID GENMASK_ULL(31, 0) #define PRIQ_0_SSID GENMASK_ULL(51, 32)
The commit f115f3c0d5d8 ("iommu/arm-smmu-v3: Decrease the queue size of evtq and priq") decreases evtq and priq, which may lead evtq/priq to be full with fault events, e.g HiSilicon ZIP/SEC/HPRE have maximum 1024 queues in one device, every queue could be binded with one process and trigger a fault event. So let's revert f115f3c0d5d8. In fact, if an implementation of SMMU really does not need so long evtq and priq, value of IDR1_EVTQS and IDR1_PRIQS can be set to proper ones. Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)