From: John Garry <john.garry@huawei.com> To: Robin Murphy <robin.murphy@arm.com>, "joro@8bytes.org" <joro@8bytes.org>, "will@kernel.org" <will@kernel.org>, "mst@redhat.com" <mst@redhat.com>, "jasowang@redhat.com" <jasowang@redhat.com> Cc: "xieyongji@bytedance.com" <xieyongji@bytedance.com>, "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>, Linuxarm <linuxarm@huawei.com> Subject: Re: [PATCH] iommu/iova: Separate out rcache init Date: Wed, 26 Jan 2022 17:58:04 +0000 [thread overview] Message-ID: <0ec8829e-aef3-eee7-17cf-416b28db3c4c@huawei.com> (raw) In-Reply-To: <ee4593b8-cdf6-935a-0eaf-48a8bfeae912@arm.com> Hi Robin, >> >> Signed-off-by: John Garry<john.garry@huawei.com> > Mangled patch? (no "---" separator here) hmm... not sure. As an experiment, I just downloaded this patch from lore.kernel.org and it applies ok. > > Overall this looks great, just a few comments further down... > ... >> +} >> +EXPORT_SYMBOL_GPL(iova_domain_init_rcaches); >> + >> +void iova_domain_free_rcaches(struct iova_domain *iovad) >> +{ >> + cpuhp_state_remove_instance_nocalls(CPUHP_IOMMU_IOVA_DEAD, >> + &iovad->cpuhp_dead); >> + free_iova_rcaches(iovad); >> } >> +EXPORT_SYMBOL_GPL(iova_domain_free_rcaches); > I think we should continue to expect external callers to clean up with > put_iova_domain(). ok, fine, makes sense > If they aren't doing that already they have a bug > (albeit minor), and we don't want to give the impression that it's OK to > free the caches at any point*other* than tearing down the whole > iova_domain, since the implementation really wouldn't expect that. > >> /* >> * Try inserting IOVA range starting with 'iova_pfn' into 'rcache', and >> @@ -831,7 +872,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad, >> { >> unsigned int log_size = order_base_2(size); >> >> - if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE) >> + if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches) >> return 0; >> .. >> @@ -102,6 +92,8 @@ struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo, >> unsigned long pfn_hi); >> void init_iova_domain(struct iova_domain *iovad, unsigned long granule, >> unsigned long start_pfn); >> +int iova_domain_init_rcaches(struct iova_domain *iovad); >> +void iova_domain_free_rcaches(struct iova_domain *iovad); > As above, I vote for just forward-declaring the free routine in iova.c > and keeping it entirely private. ok > >> struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn); >> void put_iova_domain(struct iova_domain *iovad); >> #else >> @@ -157,6 +149,15 @@ static inline void init_iova_domain(struct iova_domain *iovad, >> { >> } >> >> +static inline int iova_domain_init_rcaches(struct iova_domain *iovad) >> +{ >> + return -ENOTSUPP; >> +} >> + >> +static inline void iova_domain_free_rcaches(struct iova_domain *iovad) >> +{ >> +} >> + > I'd be inclined not to add stubs at all - I think it's a reasonable > assumption that anyone involved enough to care about rcaches has a hard > dependency on IOMMU_IOVA already. So iova_domain_free_rcaches() would disappear from here as a result of the changes discussed above. As for iova_domain_init_rcaches(), I was just following the IOMMU/IOVA coding practice - that is, always stub. > It's certainly the case today, and I'd > hardly want to encourage more users anyway. I think that stronger deterrents would be needed :) Anyway, I can remove it. Thanks, John
WARNING: multiple messages have this Message-ID (diff)
From: John Garry via iommu <iommu@lists.linux-foundation.org> To: Robin Murphy <robin.murphy@arm.com>, "joro@8bytes.org" <joro@8bytes.org>, "will@kernel.org" <will@kernel.org>, "mst@redhat.com" <mst@redhat.com>, "jasowang@redhat.com" <jasowang@redhat.com> Cc: "xieyongji@bytedance.com" <xieyongji@bytedance.com>, "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>, Linuxarm <linuxarm@huawei.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org> Subject: Re: [PATCH] iommu/iova: Separate out rcache init Date: Wed, 26 Jan 2022 17:58:04 +0000 [thread overview] Message-ID: <0ec8829e-aef3-eee7-17cf-416b28db3c4c@huawei.com> (raw) In-Reply-To: <ee4593b8-cdf6-935a-0eaf-48a8bfeae912@arm.com> Hi Robin, >> >> Signed-off-by: John Garry<john.garry@huawei.com> > Mangled patch? (no "---" separator here) hmm... not sure. As an experiment, I just downloaded this patch from lore.kernel.org and it applies ok. > > Overall this looks great, just a few comments further down... > ... >> +} >> +EXPORT_SYMBOL_GPL(iova_domain_init_rcaches); >> + >> +void iova_domain_free_rcaches(struct iova_domain *iovad) >> +{ >> + cpuhp_state_remove_instance_nocalls(CPUHP_IOMMU_IOVA_DEAD, >> + &iovad->cpuhp_dead); >> + free_iova_rcaches(iovad); >> } >> +EXPORT_SYMBOL_GPL(iova_domain_free_rcaches); > I think we should continue to expect external callers to clean up with > put_iova_domain(). ok, fine, makes sense > If they aren't doing that already they have a bug > (albeit minor), and we don't want to give the impression that it's OK to > free the caches at any point*other* than tearing down the whole > iova_domain, since the implementation really wouldn't expect that. > >> /* >> * Try inserting IOVA range starting with 'iova_pfn' into 'rcache', and >> @@ -831,7 +872,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad, >> { >> unsigned int log_size = order_base_2(size); >> >> - if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE) >> + if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches) >> return 0; >> .. >> @@ -102,6 +92,8 @@ struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo, >> unsigned long pfn_hi); >> void init_iova_domain(struct iova_domain *iovad, unsigned long granule, >> unsigned long start_pfn); >> +int iova_domain_init_rcaches(struct iova_domain *iovad); >> +void iova_domain_free_rcaches(struct iova_domain *iovad); > As above, I vote for just forward-declaring the free routine in iova.c > and keeping it entirely private. ok > >> struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn); >> void put_iova_domain(struct iova_domain *iovad); >> #else >> @@ -157,6 +149,15 @@ static inline void init_iova_domain(struct iova_domain *iovad, >> { >> } >> >> +static inline int iova_domain_init_rcaches(struct iova_domain *iovad) >> +{ >> + return -ENOTSUPP; >> +} >> + >> +static inline void iova_domain_free_rcaches(struct iova_domain *iovad) >> +{ >> +} >> + > I'd be inclined not to add stubs at all - I think it's a reasonable > assumption that anyone involved enough to care about rcaches has a hard > dependency on IOMMU_IOVA already. So iova_domain_free_rcaches() would disappear from here as a result of the changes discussed above. As for iova_domain_init_rcaches(), I was just following the IOMMU/IOVA coding practice - that is, always stub. > It's certainly the case today, and I'd > hardly want to encourage more users anyway. I think that stronger deterrents would be needed :) Anyway, I can remove it. Thanks, John _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2022-01-26 17:58 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-26 13:55 [PATCH] iommu/iova: Separate out rcache init John Garry 2022-01-26 13:55 ` John Garry via iommu 2022-01-26 17:00 ` Robin Murphy 2022-01-26 17:00 ` Robin Murphy 2022-01-26 17:00 ` Robin Murphy 2022-01-26 17:58 ` John Garry [this message] 2022-01-26 17:58 ` John Garry via iommu 2022-01-28 11:32 ` John Garry 2022-01-28 11:32 ` John Garry via iommu 2022-01-28 16:54 ` Robin Murphy 2022-01-28 16:54 ` Robin Murphy 2022-01-28 16:54 ` Robin Murphy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0ec8829e-aef3-eee7-17cf-416b28db3c4c@huawei.com \ --to=john.garry@huawei.com \ --cc=iommu@lists.linux-foundation.org \ --cc=jasowang@redhat.com \ --cc=joro@8bytes.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxarm@huawei.com \ --cc=mst@redhat.com \ --cc=robin.murphy@arm.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=will@kernel.org \ --cc=xieyongji@bytedance.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.