* Re: [PATCH] cxl/region: Move coherence tracking into cxl_region_attach()
2023-03-28 18:36 ` [PATCH] cxl/region: Move coherence tracking into cxl_region_attach() Dan Williams
@ 2023-03-29 17:40 ` Dave Jiang
2023-03-29 18:09 ` Fan Ni
2023-03-30 17:28 ` Jonathan Cameron
2 siblings, 0 replies; 6+ messages in thread
From: Dave Jiang @ 2023-03-29 17:40 UTC (permalink / raw)
To: Dan Williams, linux-cxl
Cc: vishal.l.verma, ira.weiny, alison.schofield, Jonathan.Cameron
On 3/28/23 11:36 AM, Dan Williams wrote:
> Each time the contents of a given HPA are potentially changed in a cache
> incoherent manner the CXL core sets CXL_REGION_F_INCOHERENT to
> invalidate CPU caches before the region is used.
>
> Successful invocation of attach_target() indicates that DPA has been
> newly assigned to a given HPA in the dynamic region creation flow.
> However, attach_target() is also reused in the autodiscovery flow where
> the region was activated by platform firmware. In that case there is no
> need to invalidate caches because that region is already in active use
> and nothing about the autodiscovery flow modifies the HPA-to-DPA
> relationship.
>
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/region.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 52bbf6268d5f..b2fd67fcebfb 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1674,6 +1674,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> if (rc)
> goto err_decrement;
> p->state = CXL_CONFIG_ACTIVE;
> + set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
> }
>
> cxled->cxld.interleave_ways = p->interleave_ways;
> @@ -1775,8 +1776,6 @@ static int attach_target(struct cxl_region *cxlr,
>
> down_read(&cxl_dpa_rwsem);
> rc = cxl_region_attach(cxlr, cxled, pos);
> - if (rc == 0)
> - set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
> up_read(&cxl_dpa_rwsem);
> up_write(&cxl_region_rwsem);
> return rc;
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cxl/region: Move coherence tracking into cxl_region_attach()
2023-03-28 18:36 ` [PATCH] cxl/region: Move coherence tracking into cxl_region_attach() Dan Williams
2023-03-29 17:40 ` Dave Jiang
@ 2023-03-29 18:09 ` Fan Ni
2023-03-30 17:28 ` Jonathan Cameron
2 siblings, 0 replies; 6+ messages in thread
From: Fan Ni @ 2023-03-29 18:09 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl@vger.kernel.org, vishal.l.verma@intel.com,
ira.weiny@intel.com, dave.jiang@intel.com,
alison.schofield@intel.com, Jonathan.Cameron@huawei.com
On Tue, Mar 28, 2023 at 11:36:28AM -0700, Dan Williams wrote:
> Each time the contents of a given HPA are potentially changed in a cache
> incoherent manner the CXL core sets CXL_REGION_F_INCOHERENT to
> invalidate CPU caches before the region is used.
>
> Successful invocation of attach_target() indicates that DPA has been
> newly assigned to a given HPA in the dynamic region creation flow.
> However, attach_target() is also reused in the autodiscovery flow where
> the region was activated by platform firmware. In that case there is no
> need to invalidate caches because that region is already in active use
> and nothing about the autodiscovery flow modifies the HPA-to-DPA
> relationship.
>
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
Reviewed-by: Fan Ni <fan.ni@samsung.com>
> drivers/cxl/core/region.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 52bbf6268d5f..b2fd67fcebfb 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1674,6 +1674,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> if (rc)
> goto err_decrement;
> p->state = CXL_CONFIG_ACTIVE;
> + set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
> }
>
> cxled->cxld.interleave_ways = p->interleave_ways;
> @@ -1775,8 +1776,6 @@ static int attach_target(struct cxl_region *cxlr,
>
> down_read(&cxl_dpa_rwsem);
> rc = cxl_region_attach(cxlr, cxled, pos);
> - if (rc == 0)
> - set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
> up_read(&cxl_dpa_rwsem);
> up_write(&cxl_region_rwsem);
> return rc;
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cxl/region: Move coherence tracking into cxl_region_attach()
2023-03-28 18:36 ` [PATCH] cxl/region: Move coherence tracking into cxl_region_attach() Dan Williams
2023-03-29 17:40 ` Dave Jiang
2023-03-29 18:09 ` Fan Ni
@ 2023-03-30 17:28 ` Jonathan Cameron
2023-03-30 18:16 ` Dan Williams
2 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2023-03-30 17:28 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, vishal.l.verma, ira.weiny, dave.jiang,
alison.schofield
On Tue, 28 Mar 2023 11:36:28 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Each time the contents of a given HPA are potentially changed in a cache
> incoherent manner the CXL core sets CXL_REGION_F_INCOHERENT to
> invalidate CPU caches before the region is used.
>
> Successful invocation of attach_target() indicates that DPA has been
> newly assigned to a given HPA in the dynamic region creation flow.
> However, attach_target() is also reused in the autodiscovery flow where
> the region was activated by platform firmware. In that case there is no
> need to invalidate caches because that region is already in active use
> and nothing about the autodiscovery flow modifies the HPA-to-DPA
> relationship.
>
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Looks correct to me. Might be worth making it clear in the patch description
that for auto discovered regions, we have exited cxl_region_attach()
before the point where you set the flag.
I'm also fairly sure that in previous flow the cache flush was happening on
each target being added rather than just on the final one being added which
would be a significant overhead also avoided via this patch.
Jonathan
> ---
> drivers/cxl/core/region.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 52bbf6268d5f..b2fd67fcebfb 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1674,6 +1674,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> if (rc)
> goto err_decrement;
> p->state = CXL_CONFIG_ACTIVE;
> + set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
> }
>
> cxled->cxld.interleave_ways = p->interleave_ways;
> @@ -1775,8 +1776,6 @@ static int attach_target(struct cxl_region *cxlr,
>
> down_read(&cxl_dpa_rwsem);
> rc = cxl_region_attach(cxlr, cxled, pos);
> - if (rc == 0)
> - set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
> up_read(&cxl_dpa_rwsem);
> up_write(&cxl_region_rwsem);
> return rc;
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cxl/region: Move coherence tracking into cxl_region_attach()
2023-03-30 17:28 ` Jonathan Cameron
@ 2023-03-30 18:16 ` Dan Williams
2023-03-30 18:25 ` Jonathan Cameron
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2023-03-30 18:16 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, vishal.l.verma, ira.weiny, dave.jiang,
alison.schofield
Jonathan Cameron wrote:
> On Tue, 28 Mar 2023 11:36:28 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Each time the contents of a given HPA are potentially changed in a cache
> > incoherent manner the CXL core sets CXL_REGION_F_INCOHERENT to
> > invalidate CPU caches before the region is used.
> >
> > Successful invocation of attach_target() indicates that DPA has been
> > newly assigned to a given HPA in the dynamic region creation flow.
> > However, attach_target() is also reused in the autodiscovery flow where
> > the region was activated by platform firmware. In that case there is no
> > need to invalidate caches because that region is already in active use
> > and nothing about the autodiscovery flow modifies the HPA-to-DPA
> > relationship.
> >
> > Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Looks correct to me. Might be worth making it clear in the patch description
> that for auto discovered regions, we have exited cxl_region_attach()
> before the point where you set the flag.
Sure, added:
"In the autodiscovery case cxl_region_attach() exits early after
determining the endpoint decoder is already correctly attached to the
region."
> I'm also fairly sure that in previous flow the cache flush was happening on
> each target being added rather than just on the final one being added which
> would be a significant overhead also avoided via this patch.
No, it did set the bit multiple times, but the actual flush itself does
not occur until the region is activated in cxl_region_probe() (the only
caller of cxl_region_invalidate_memregion()).
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cxl/region: Move coherence tracking into cxl_region_attach()
2023-03-30 18:16 ` Dan Williams
@ 2023-03-30 18:25 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2023-03-30 18:25 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, vishal.l.verma, ira.weiny, dave.jiang,
alison.schofield
On Thu, 30 Mar 2023 11:16:42 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Jonathan Cameron wrote:
> > On Tue, 28 Mar 2023 11:36:28 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > > Each time the contents of a given HPA are potentially changed in a cache
> > > incoherent manner the CXL core sets CXL_REGION_F_INCOHERENT to
> > > invalidate CPU caches before the region is used.
> > >
> > > Successful invocation of attach_target() indicates that DPA has been
> > > newly assigned to a given HPA in the dynamic region creation flow.
> > > However, attach_target() is also reused in the autodiscovery flow where
> > > the region was activated by platform firmware. In that case there is no
> > > need to invalidate caches because that region is already in active use
> > > and nothing about the autodiscovery flow modifies the HPA-to-DPA
> > > relationship.
> > >
> > > Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >
> > Looks correct to me. Might be worth making it clear in the patch description
> > that for auto discovered regions, we have exited cxl_region_attach()
> > before the point where you set the flag.
>
> Sure, added:
>
> "In the autodiscovery case cxl_region_attach() exits early after
> determining the endpoint decoder is already correctly attached to the
> region."
Thanks.
>
>
> > I'm also fairly sure that in previous flow the cache flush was happening on
> > each target being added rather than just on the final one being added which
> > would be a significant overhead also avoided via this patch.
>
> No, it did set the bit multiple times, but the actual flush itself does
> not occur until the region is activated in cxl_region_probe() (the only
> caller of cxl_region_invalidate_memregion()).
ah. That makes sense. Bit isn't magic :)
^ permalink raw reply [flat|nested] 6+ messages in thread