diff mbox

[v2,10/11] PCI: Stop caching ATS Invalidate Queue Depth

Message ID 20150721001511.28145.29144.stgit@bhelgaas-glaptop2.roam.corp.google.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Bjorn Helgaas July 21, 2015, 12:15 a.m. UTC 
Stop caching the Invalidate Queue Depth in struct pci_dev.
pci_ats_queue_depth() is typically called only once per device, and it
returns a fixed value per-device, so callers who need the value frequently
can cache it themselves.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c   |    9 ++++-----
 include/linux/pci.h |    1 -
 2 files changed, 4 insertions(+), 6 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Joerg Roedel July 27, 2015, 12:57 p.m. UTC | #1 
On Mon, Jul 20, 2015 at 07:15:11PM -0500, Bjorn Helgaas wrote:
> Stop caching the Invalidate Queue Depth in struct pci_dev.
> pci_ats_queue_depth() is typically called only once per device, and it
> returns a fixed value per-device, so callers who need the value frequently
> can cache it themselves.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c   |    9 ++++-----
>  include/linux/pci.h |    1 -
>  2 files changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Joerg Roedel <jroedel@suse.de>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Don Dutile July 27, 2015, 2 p.m. UTC | #2 
On 07/20/2015 08:15 PM, Bjorn Helgaas wrote:
> Stop caching the Invalidate Queue Depth in struct pci_dev.
> pci_ats_queue_depth() is typically called only once per device, and it
> returns a fixed value per-device, so callers who need the value frequently
> can cache it themselves.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/ats.c   |    9 ++++-----
>   include/linux/pci.h |    1 -
>   2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 67524a7..bdb1383 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -20,16 +20,12 @@
>   void pci_ats_init(struct pci_dev *dev)
>   {
>   	int pos;
> -	u16 cap;
>
>   	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
>   	if (!pos)
>   		return;
>
>   	dev->ats_cap = pos;
> -	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
> -	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
> -					    PCI_ATS_MAX_QDEP;
>   }
>
>   /**
> @@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
>    */
>   int pci_ats_queue_depth(struct pci_dev *dev)
>   {
> +	u16 cap;
> +
>   	if (!dev->ats_cap)
>   		return -EINVAL;
>
>   	if (dev->is_virtfn)
>   		return 0;
>
hmmm, isn't one of the fixes in this patch set to
change the caching of ats queue depth ?  won't above
make it 0 for all VF's ?  is that correct, or desired (virtual) value?

> -	return dev->ats_qdep;
> +	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
> +	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP;
>   }
>   EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 307f96a..4b484fd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -378,7 +378,6 @@ struct pci_dev {
>   	};
>   	u16		ats_cap;	/* ATS Capability offset */
>   	u8		ats_stu;	/* ATS Smallest Translation Unit */
> -	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
>   	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
>   #endif
>   	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas July 27, 2015, 10:27 p.m. UTC | #3 
Hi Don,

On Mon, Jul 27, 2015 at 10:00:53AM -0400, Don Dutile wrote:
> On 07/20/2015 08:15 PM, Bjorn Helgaas wrote:
> >Stop caching the Invalidate Queue Depth in struct pci_dev.
> >pci_ats_queue_depth() is typically called only once per device, and it
> >returns a fixed value per-device, so callers who need the value frequently
> >can cache it themselves.
> >
> >Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >---
> >  drivers/pci/ats.c   |    9 ++++-----
> >  include/linux/pci.h |    1 -
> >  2 files changed, 4 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> >index 67524a7..bdb1383 100644
> >--- a/drivers/pci/ats.c
> >+++ b/drivers/pci/ats.c
> >@@ -20,16 +20,12 @@
> >  void pci_ats_init(struct pci_dev *dev)
> >  {
> >  	int pos;
> >-	u16 cap;
> >
> >  	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
> >  	if (!pos)
> >  		return;
> >
> >  	dev->ats_cap = pos;
> >-	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
> >-	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
> >-					    PCI_ATS_MAX_QDEP;
> >  }
> >
> >  /**
> >@@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
> >   */
> >  int pci_ats_queue_depth(struct pci_dev *dev)
> >  {
> >+	u16 cap;
> >+
> >  	if (!dev->ats_cap)
> >  		return -EINVAL;
> >
> >  	if (dev->is_virtfn)
> >  		return 0;
> >
> hmmm, isn't one of the fixes in this patch set to
> change the caching of ats queue depth ?  won't above
> make it 0 for all VF's ?  is that correct, or desired (virtual) value?

Per spec (SR-IOV r1.1., sec 3.7.4), the ATS queue depth register on a VF
always contains zero.

Here's the v4.1 code:

    int pci_ats_queue_depth(struct pci_dev *dev)
    {
        if (dev->is_virtfn)
                return 0;

        if (dev->ats)
                return dev->ats->qdep;

	...


In v4.1, pci_ats_queue_depth() always returned 0 for a VF.  For VFs, we
didn't look at the dev->ats->qdep cache.  So I don't think this changes
anything for the caller.

A previous patch changed this path so we return -EINVAL instead of 0 for
VFs that don't support ATS.  I think the previous behavior there was wrong,
but I doubt anybody noticed.

Bjorn

> >-	return dev->ats_qdep;
> >+	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
> >+	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP;
> >  }
> >  EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
> >
> >diff --git a/include/linux/pci.h b/include/linux/pci.h
> >index 307f96a..4b484fd 100644
> >--- a/include/linux/pci.h
> >+++ b/include/linux/pci.h
> >@@ -378,7 +378,6 @@ struct pci_dev {
> >  	};
> >  	u16		ats_cap;	/* ATS Capability offset */
> >  	u8		ats_stu;	/* ATS Smallest Translation Unit */
> >-	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
> >  	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
> >  #endif
> >  	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Don Dutile July 27, 2015, 11:13 p.m. UTC | #4 
On 07/27/2015 06:27 PM, Bjorn Helgaas wrote:
> Hi Don,
>
> On Mon, Jul 27, 2015 at 10:00:53AM -0400, Don Dutile wrote:
>> On 07/20/2015 08:15 PM, Bjorn Helgaas wrote:
>>> Stop caching the Invalidate Queue Depth in struct pci_dev.
>>> pci_ats_queue_depth() is typically called only once per device, and it
>>> returns a fixed value per-device, so callers who need the value frequently
>>> can cache it themselves.
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>>   drivers/pci/ats.c   |    9 ++++-----
>>>   include/linux/pci.h |    1 -
>>>   2 files changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>> index 67524a7..bdb1383 100644
>>> --- a/drivers/pci/ats.c
>>> +++ b/drivers/pci/ats.c
>>> @@ -20,16 +20,12 @@
>>>   void pci_ats_init(struct pci_dev *dev)
>>>   {
>>>   	int pos;
>>> -	u16 cap;
>>>
>>>   	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
>>>   	if (!pos)
>>>   		return;
>>>
>>>   	dev->ats_cap = pos;
>>> -	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
>>> -	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
>>> -					    PCI_ATS_MAX_QDEP;
>>>   }
>>>
>>>   /**
>>> @@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
>>>    */
>>>   int pci_ats_queue_depth(struct pci_dev *dev)
>>>   {
>>> +	u16 cap;
>>> +
>>>   	if (!dev->ats_cap)
>>>   		return -EINVAL;
>>>
>>>   	if (dev->is_virtfn)
>>>   		return 0;
>>>
>> hmmm, isn't one of the fixes in this patch set to
>> change the caching of ats queue depth ?  won't above
>> make it 0 for all VF's ?  is that correct, or desired (virtual) value?
>
> Per spec (SR-IOV r1.1., sec 3.7.4), the ATS queue depth register on a VF
> always contains zero.
>
> Here's the v4.1 code:
>
>      int pci_ats_queue_depth(struct pci_dev *dev)
>      {
>          if (dev->is_virtfn)
>                  return 0;
>
>          if (dev->ats)
>                  return dev->ats->qdep;
>
> 	...
>
>
> In v4.1, pci_ats_queue_depth() always returned 0 for a VF.  For VFs, we
> didn't look at the dev->ats->qdep cache.  So I don't think this changes
> anything for the caller.
>
> A previous patch changed this path so we return -EINVAL instead of 0 for
> VFs that don't support ATS.  I think the previous behavior there was wrong,
> but I doubt anybody noticed.
>
> Bjorn
>
ok. thanks for the sanity check.

>>> -	return dev->ats_qdep;
>>> +	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
>>> +	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP;
>>>   }
>>>   EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
>>>
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 307f96a..4b484fd 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -378,7 +378,6 @@ struct pci_dev {
>>>   	};
>>>   	u16		ats_cap;	/* ATS Capability offset */
>>>   	u8		ats_stu;	/* ATS Smallest Translation Unit */
>>> -	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
>>>   	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
>>>   #endif
>>>   	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 67524a7..bdb1383 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -20,16 +20,12 @@ 
 void pci_ats_init(struct pci_dev *dev)
 {
 	int pos;
-	u16 cap;
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
 	if (!pos)
 		return;
 
 	dev->ats_cap = pos;
-	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
-	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
-					    PCI_ATS_MAX_QDEP;
 }
 
 /**
@@ -131,13 +127,16 @@  EXPORT_SYMBOL_GPL(pci_restore_ats_state);
  */
 int pci_ats_queue_depth(struct pci_dev *dev)
 {
+	u16 cap;
+
 	if (!dev->ats_cap)
 		return -EINVAL;
 
 	if (dev->is_virtfn)
 		return 0;
 
-	return dev->ats_qdep;
+	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
+	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP;
 }
 EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 307f96a..4b484fd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -378,7 +378,6 @@  struct pci_dev {
 	};
 	u16		ats_cap;	/* ATS Capability offset */
 	u8		ats_stu;	/* ATS Smallest Translation Unit */
-	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
 	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
 #endif
 	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */