[PATCH 2/2 v2] PCI/P2PDMA: Modify p2p_dma_distance to detect P2P links

Shivasharan S posted 2 patches 2 months, 1 week ago
[PATCH 2/2 v2] PCI/P2PDMA: Modify p2p_dma_distance to detect P2P links
Posted by Shivasharan S 2 months, 1 week ago
Update the p2p_dma_distance() to determine inter-switch P2P links existing
between two switches and use this to calculate the DMA distance between
two devices.

Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
---
 drivers/pci/p2pdma.c       | 17 ++++++++++++++++-
 drivers/pci/pcie/portdrv.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/pci/pcie/portdrv.h |  2 ++
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 4f47a13cb500..eed3b69e7293 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -21,6 +21,8 @@
 #include <linux/seq_buf.h>
 #include <linux/xarray.h>
 
+extern bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b);
+
 struct pci_p2pdma {
 	struct gen_pool *pool;
 	bool p2pmem_published;
@@ -576,7 +578,7 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 		int *dist, bool verbose)
 {
 	enum pci_p2pdma_map_type map_type = PCI_P2PDMA_MAP_THRU_HOST_BRIDGE;
-	struct pci_dev *a = provider, *b = client, *bb;
+	struct pci_dev *a = provider, *b = client, *bb, *b_p2p_link = NULL;
 	bool acs_redirects = false;
 	struct pci_p2pdma *p2pdma;
 	struct seq_buf acs_list;
@@ -606,6 +608,16 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 			if (a == bb)
 				goto check_b_path_acs;
 
+			/*
+			 * If both upstream bridges have Inter switch P2P link
+			 * available, P2P DMA distance can account for optimized
+			 * path.
+			 */
+			if (pcie_port_is_p2p_link_available(a, bb)) {
+				b_p2p_link = bb;
+				goto check_b_path_acs;
+			}
+
 			bb = pci_upstream_bridge(bb);
 			dist_b++;
 		}
@@ -629,6 +641,9 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 			acs_cnt++;
 		}
 
+		if (bb == b_p2p_link)
+			break;
+
 		bb = pci_upstream_bridge(bb);
 	}
 
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index c940b4b242fd..2fe1598fc684 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -104,6 +104,40 @@ static bool pcie_port_is_p2p_supported(struct pci_dev *dev)
 	return false;
 }
 
+/**
+ * pcie_port_is_p2p_link_available: Determine if a P2P link is available
+ * between the two upstream bridges. The serial number of the two devices
+ * will be compared and if they are same then it is considered that the P2P
+ * link is available.
+ *
+ * Return value: true if inter switch P2P is available, return false otherwise.
+ */
+bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b)
+{
+	u64 dsn_a, dsn_b;
+
+	/*
+	 * Check if the devices support Inter switch P2P.
+	 */
+	if (!pcie_port_is_p2p_supported(a) ||
+	    !pcie_port_is_p2p_supported(b))
+		return false;
+
+	dsn_a = pci_get_dsn(a);
+	if (!dsn_a)
+		return false;
+
+	dsn_b = pci_get_dsn(b);
+	if (!dsn_b)
+		return false;
+
+	if (dsn_a == dsn_b)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(pcie_port_is_p2p_link_available);
+
 /*
  * Traverse list of all PCI bridges and find devices that support Inter switch P2P
  * and have the same serial number to create report the BDF over sysfs.
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index 1be06cb45665..b341aad6eb49 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -130,5 +130,7 @@ static inline bool pcie_pme_no_msi(void) { return false; }
 static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {}
 #endif /* !CONFIG_PCIE_PME */
 
+bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b);
+
 struct device *pcie_port_find_device(struct pci_dev *dev, u32 service);
 #endif /* _PORTDRV_H_ */
-- 
2.43.0
Re: [PATCH 2/2 v2] PCI/P2PDMA: Modify p2p_dma_distance to detect P2P links
Posted by Jonathan Cameron 2 months ago
On Thu, 19 Sep 2024 01:13:44 -0700
Shivasharan S <shivasharan.srikanteshwara@broadcom.com> wrote:

> Update the p2p_dma_distance() to determine inter-switch P2P links existing
> between two switches and use this to calculate the DMA distance between
> two devices.
> 
> Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
> ---
>  drivers/pci/p2pdma.c       | 17 ++++++++++++++++-
>  drivers/pci/pcie/portdrv.c | 34 ++++++++++++++++++++++++++++++++++
>  drivers/pci/pcie/portdrv.h |  2 ++
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 4f47a13cb500..eed3b69e7293 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -21,6 +21,8 @@
>  #include <linux/seq_buf.h>
>  #include <linux/xarray.h>
>  
> +extern bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b);

That's nasty.  Include the header so you get a clean stub if
this support is not built in etc.

> +

> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index c940b4b242fd..2fe1598fc684 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -104,6 +104,40 @@ static bool pcie_port_is_p2p_supported(struct pci_dev *dev)
>  	return false;
>  }
>  
> +/**
> + * pcie_port_is_p2p_link_available: Determine if a P2P link is available
> + * between the two upstream bridges. The serial number of the two devices
> + * will be compared and if they are same then it is considered that the P2P
> + * link is available.
> + *
> + * Return value: true if inter switch P2P is available, return false otherwise.
> + */
> +bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b)
> +{
> +	u64 dsn_a, dsn_b;
> +
> +	/*
> +	 * Check if the devices support Inter switch P2P.
> +	 */

Single line comment syntax fine here.  However it's kind
of obvious, so I'd just drop the comment.


> +	if (!pcie_port_is_p2p_supported(a) ||
> +	    !pcie_port_is_p2p_supported(b))

Don't wrap this. I think it's under 80 chars anyway.

> +		return false;
> +
> +	dsn_a = pci_get_dsn(a);
> +	if (!dsn_a)
> +		return false;
If we assume that dsn is the only right way to detect this
(I'm fine with that for now) then we know the supported tests
above would only pass if this is true. Hence

return pci_get_dsn(a) == pci_get_dsn(b);

should be fine.

> +
> +	dsn_b = pci_get_dsn(b);
> +	if (!dsn_b)
> +		return false;
> +
> +	if (dsn_a == dsn_b)
> +		return true;

	return dsn_a == dsn_b;

> +
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(pcie_port_is_p2p_link_available);
> +
>  /*
>   * Traverse list of all PCI bridges and find devices that support Inter switch P2P
>   * and have the same serial number to create report the BDF over sysfs.
Re: [PATCH 2/2 v2] PCI/P2PDMA: Modify p2p_dma_distance to detect P2P links
Posted by Shivasharan Srikanteshwara 1 month, 2 weeks ago
On Tue, Sep 24, 2024 at 8:38 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 19 Sep 2024 01:13:44 -0700
> Shivasharan S <shivasharan.srikanteshwara@broadcom.com> wrote:
>
> > Update the p2p_dma_distance() to determine inter-switch P2P links existing
> > between two switches and use this to calculate the DMA distance between
> > two devices.
> >
> > Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
> > ---
> >  drivers/pci/p2pdma.c       | 17 ++++++++++++++++-
> >  drivers/pci/pcie/portdrv.c | 34 ++++++++++++++++++++++++++++++++++
> >  drivers/pci/pcie/portdrv.h |  2 ++
> >  3 files changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> > index 4f47a13cb500..eed3b69e7293 100644
> > --- a/drivers/pci/p2pdma.c
> > +++ b/drivers/pci/p2pdma.c
> > @@ -21,6 +21,8 @@
> >  #include <linux/seq_buf.h>
> >  #include <linux/xarray.h>
> >
> > +extern bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b);
>
> That's nasty.  Include the header so you get a clean stub if
> this support is not built in etc.
>
Will move this to the new header file that will be added.
> > +
>
> > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> > index c940b4b242fd..2fe1598fc684 100644
> > --- a/drivers/pci/pcie/portdrv.c
> > +++ b/drivers/pci/pcie/portdrv.c
> > @@ -104,6 +104,40 @@ static bool pcie_port_is_p2p_supported(struct pci_dev *dev)
> >       return false;
> >  }
> >
> > +/**
> > + * pcie_port_is_p2p_link_available: Determine if a P2P link is available
> > + * between the two upstream bridges. The serial number of the two devices
> > + * will be compared and if they are same then it is considered that the P2P
> > + * link is available.
> > + *
> > + * Return value: true if inter switch P2P is available, return false otherwise.
> > + */
> > +bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b)
> > +{
> > +     u64 dsn_a, dsn_b;
> > +
> > +     /*
> > +      * Check if the devices support Inter switch P2P.
> > +      */
>
> Single line comment syntax fine here.  However it's kind
> of obvious, so I'd just drop the comment.
>
>
Will do.

> > +     if (!pcie_port_is_p2p_supported(a) ||
> > +         !pcie_port_is_p2p_supported(b))
>
> Don't wrap this. I think it's under 80 chars anyway.
>
Will do.

> > +             return false;
> > +
> > +     dsn_a = pci_get_dsn(a);
> > +     if (!dsn_a)
> > +             return false;
> If we assume that dsn is the only right way to detect this
> (I'm fine with that for now) then we know the supported tests
> above would only pass if this is true. Hence
>
> return pci_get_dsn(a) == pci_get_dsn(b);
>
> should be fine.
>
Agreed. Will rework this as suggested and update in the next
patch.

> > +
> > +     dsn_b = pci_get_dsn(b);
> > +     if (!dsn_b)
> > +             return false;
> > +
> > +     if (dsn_a == dsn_b)
> > +             return true;
>
>         return dsn_a == dsn_b;
>
Above changes will take care of this as well.

> > +
> > +     return false;
> > +}
> > +EXPORT_SYMBOL_GPL(pcie_port_is_p2p_link_available);
> > +
> >  /*
> >   * Traverse list of all PCI bridges and find devices that support Inter switch P2P
> >   * and have the same serial number to create report the BDF over sysfs.
>
>
Re: [PATCH 2/2 v2] PCI/P2PDMA: Modify p2p_dma_distance to detect P2P links
Posted by kernel test robot 2 months, 1 week ago
Hi Shivasharan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on next-20240919]
[cannot apply to pci/for-linus linus/master v6.11]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shivasharan-S/PCI-portdrv-Enable-reporting-inter-switch-P2P-links/20240919-162626
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/1726733624-2142-3-git-send-email-shivasharan.srikanteshwara%40broadcom.com
patch subject: [PATCH 2/2 v2] PCI/P2PDMA: Modify p2p_dma_distance to detect P2P links
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20240920/202409201333.LCx5k41f-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240920/202409201333.LCx5k41f-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409201333.LCx5k41f-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/pci/pcie/portdrv.c:86: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * Determine if device supports Inter switch P2P links.
>> drivers/pci/pcie/portdrv.c:116: warning: Function parameter or struct member 'a' not described in 'pcie_port_is_p2p_link_available'
>> drivers/pci/pcie/portdrv.c:116: warning: Function parameter or struct member 'b' not described in 'pcie_port_is_p2p_link_available'


vim +116 drivers/pci/pcie/portdrv.c

   106	
   107	/**
   108	 * pcie_port_is_p2p_link_available: Determine if a P2P link is available
   109	 * between the two upstream bridges. The serial number of the two devices
   110	 * will be compared and if they are same then it is considered that the P2P
   111	 * link is available.
   112	 *
   113	 * Return value: true if inter switch P2P is available, return false otherwise.
   114	 */
   115	bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b)
 > 116	{
   117		u64 dsn_a, dsn_b;
   118	
   119		/*
   120		 * Check if the devices support Inter switch P2P.
   121		 */
   122		if (!pcie_port_is_p2p_supported(a) ||
   123		    !pcie_port_is_p2p_supported(b))
   124			return false;
   125	
   126		dsn_a = pci_get_dsn(a);
   127		if (!dsn_a)
   128			return false;
   129	
   130		dsn_b = pci_get_dsn(b);
   131		if (!dsn_b)
   132			return false;
   133	
   134		if (dsn_a == dsn_b)
   135			return true;
   136	
   137		return false;
   138	}
   139	EXPORT_SYMBOL_GPL(pcie_port_is_p2p_link_available);
   140	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki