diff mbox

[dpdk-dev] pci/uio: enable prefetchable resources mapping

Message ID 1496530644-8393-1-git-send-email-changpeng.liu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Liu, Changpeng June 3, 2017, 10:57 p.m. UTC 
  For PCI prefetchable resources, Linux will create a
write combined file as well, the library will try
to map resourceX_wc file first, if the file does
not exist, then it will map resourceX as usual.

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Ferruh Yigit Oct. 5, 2017, 12:06 a.m. UTC | #1 
On 6/3/2017 11:57 PM, Changpeng Liu wrote:
> For PCI prefetchable resources, Linux will create a
> write combined file as well, the library will try
> to map resourceX_wc file first, if the file does
> not exist, then it will map resourceX as usual.

Hi Changpeng,

Code part looks OK, but can you please describe more why we should try
write combined resource file first, what is the benefit of using it _wc
file?

Thanks,
ferruh


> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index fa10329..d9fc20a 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -321,7 +321,7 @@
>  
>  	/* update devname for mmap  */
>  	snprintf(devname, sizeof(devname),
> -			"%s/" PCI_PRI_FMT "/resource%d",
> +			"%s/" PCI_PRI_FMT "/resource%d_wc",
>  			pci_get_sysfs_path(),
>  			loc->domain, loc->bus, loc->devid,
>  			loc->function, res_idx);
> @@ -335,13 +335,22 @@
>  	}
>  
>  	/*
> -	 * open resource file, to mmap it
> +	 * open prefetchable resource file first, try to mmap it
>  	 */
>  	fd = open(devname, O_RDWR);
>  	if (fd < 0) {
> -		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> -				devname, strerror(errno));
> -		goto error;
> +		snprintf(devname, sizeof(devname),
> +				"%s/" PCI_PRI_FMT "/resource%d",
> +				pci_get_sysfs_path(),
> +				loc->domain, loc->bus, loc->devid,
> +				loc->function, res_idx);
> +		/* then try to map resource file */
> +		fd = open(devname, O_RDWR);
> +		if (fd < 0) {
> +			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> +					devname, strerror(errno));
> +			goto error;
> +		}
>  	}
>  
>  	/* try mapping somewhere close to the end of hugepages */
>
Bruce Richardson Oct. 5, 2017, 8:28 a.m. UTC | #2 
On Thu, Oct 05, 2017 at 01:06:41AM +0100, Ferruh Yigit wrote:
> On 6/3/2017 11:57 PM, Changpeng Liu wrote:
> > For PCI prefetchable resources, Linux will create a
> > write combined file as well, the library will try
> > to map resourceX_wc file first, if the file does
> > not exist, then it will map resourceX as usual.
> 
> Hi Changpeng,
> 
> Code part looks OK, but can you please describe more why we should try
> write combined resource file first, what is the benefit of using it _wc
> file?
> 
> Thanks,
> ferruh
> 
Also, if we use a write combining resource file, I believe we will use
correct ordering of instructions within our drivers. Does applying this
patch not also mean that we would need memory barriers inside all the
drivers, so as to ensure that we don't have a queue doorbell write
reordered with the descriptor writes? I don't think it's safe to apply
this change on it's own, without driver changes, since all PMDs assume
strong ordering on IA.

Regards,
/Bruce

> 
> > 
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > index fa10329..d9fc20a 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > @@ -321,7 +321,7 @@
> >  
> >  	/* update devname for mmap  */
> >  	snprintf(devname, sizeof(devname),
> > -			"%s/" PCI_PRI_FMT "/resource%d",
> > +			"%s/" PCI_PRI_FMT "/resource%d_wc",
> >  			pci_get_sysfs_path(),
> >  			loc->domain, loc->bus, loc->devid,
> >  			loc->function, res_idx);
> > @@ -335,13 +335,22 @@
> >  	}
> >  
> >  	/*
> > -	 * open resource file, to mmap it
> > +	 * open prefetchable resource file first, try to mmap it
> >  	 */
> >  	fd = open(devname, O_RDWR);
> >  	if (fd < 0) {
> > -		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> > -				devname, strerror(errno));
> > -		goto error;
> > +		snprintf(devname, sizeof(devname),
> > +				"%s/" PCI_PRI_FMT "/resource%d",
> > +				pci_get_sysfs_path(),
> > +				loc->domain, loc->bus, loc->devid,
> > +				loc->function, res_idx);
> > +		/* then try to map resource file */
> > +		fd = open(devname, O_RDWR);
> > +		if (fd < 0) {
> > +			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> > +					devname, strerror(errno));
> > +			goto error;
> > +		}
> >  	}
> >  
> >  	/* try mapping somewhere close to the end of hugepages */
> > 
>
Bruce Richardson Oct. 5, 2017, 8:33 a.m. UTC | #3 
On Thu, Oct 05, 2017 at 09:28:34AM +0100, Bruce Richardson wrote:
> On Thu, Oct 05, 2017 at 01:06:41AM +0100, Ferruh Yigit wrote:
> > On 6/3/2017 11:57 PM, Changpeng Liu wrote:
> > > For PCI prefetchable resources, Linux will create a
> > > write combined file as well, the library will try
> > > to map resourceX_wc file first, if the file does
> > > not exist, then it will map resourceX as usual.
> > 
> > Hi Changpeng,
> > 
> > Code part looks OK, but can you please describe more why we should try
> > write combined resource file first, what is the benefit of using it _wc
> > file?
> > 
> > Thanks,
> > ferruh
> > 
> Also, if we use a write combining resource file, I believe we will use
s/will use/will lose/

> correct ordering of instructions within our drivers. Does applying this
> patch not also mean that we would need memory barriers inside all the
> drivers, so as to ensure that we don't have a queue doorbell write
> reordered with the descriptor writes? I don't think it's safe to apply
> this change on it's own, without driver changes, since all PMDs assume
> strong ordering on IA.
> 
> Regards,
> /Bruce
> 
> > 
> > > 
> > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > > ---
> > >  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 19 ++++++++++++++-----
> > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > > index fa10329..d9fc20a 100644
> > > --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > > @@ -321,7 +321,7 @@
> > >  
> > >  	/* update devname for mmap  */
> > >  	snprintf(devname, sizeof(devname),
> > > -			"%s/" PCI_PRI_FMT "/resource%d",
> > > +			"%s/" PCI_PRI_FMT "/resource%d_wc",
> > >  			pci_get_sysfs_path(),
> > >  			loc->domain, loc->bus, loc->devid,
> > >  			loc->function, res_idx);
> > > @@ -335,13 +335,22 @@
> > >  	}
> > >  
> > >  	/*
> > > -	 * open resource file, to mmap it
> > > +	 * open prefetchable resource file first, try to mmap it
> > >  	 */
> > >  	fd = open(devname, O_RDWR);
> > >  	if (fd < 0) {
> > > -		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> > > -				devname, strerror(errno));
> > > -		goto error;
> > > +		snprintf(devname, sizeof(devname),
> > > +				"%s/" PCI_PRI_FMT "/resource%d",
> > > +				pci_get_sysfs_path(),
> > > +				loc->domain, loc->bus, loc->devid,
> > > +				loc->function, res_idx);
> > > +		/* then try to map resource file */
> > > +		fd = open(devname, O_RDWR);
> > > +		if (fd < 0) {
> > > +			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> > > +					devname, strerror(errno));
> > > +			goto error;
> > > +		}
> > >  	}
> > >  
> > >  	/* try mapping somewhere close to the end of hugepages */
> > > 
> >
Ferruh Yigit Oct. 28, 2018, 1:59 a.m. UTC | #4 
On 10/5/2017 9:33 AM, bruce.richardson at intel.com (Bruce Richardson) wrote:
> On Thu, Oct 05, 2017 at 09:28:34AM +0100, Bruce Richardson wrote:
>> On Thu, Oct 05, 2017 at 01:06:41AM +0100, Ferruh Yigit wrote:
>>> On 6/3/2017 11:57 PM, Changpeng Liu wrote:
>>>> For PCI prefetchable resources, Linux will create a
>>>> write combined file as well, the library will try
>>>> to map resourceX_wc file first, if the file does
>>>> not exist, then it will map resourceX as usual.
>>>
>>> Hi Changpeng,
>>>
>>> Code part looks OK, but can you please describe more why we should try
>>> write combined resource file first, what is the benefit of using it _wc
>>> file?
>>>
>>> Thanks,
>>> ferruh
>>>
>> Also, if we use a write combining resource file, I believe we will use
> s/will use/will lose/
> 
>> correct ordering of instructions within our drivers. Does applying this
>> patch not also mean that we would need memory barriers inside all the
>> drivers, so as to ensure that we don't have a queue doorbell write
>> reordered with the descriptor writes? I don't think it's safe to apply
>> this change on it's own, without driver changes, since all PMDs assume
>> strong ordering on IA.
>>
>> Regards,
>> /Bruce

Hi Changpeng,

Following patch looks like superseded this patch, can you please check it. I
will update the status of this patch in patchwork.

https://patches.dpdk.org/project/dpdk/list/?series=323&state=%2A&archive=both
https://patches.dpdk.org/patch/41971/


>>
>>>
>>>>
>>>> Signed-off-by: Changpeng Liu <changpeng.liu at intel.com>
>>>> ---
>>>>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 19 ++++++++++++++-----
>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>>>> index fa10329..d9fc20a 100644
>>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>>>> @@ -321,7 +321,7 @@
>>>>  
>>>>  	/* update devname for mmap  */
>>>>  	snprintf(devname, sizeof(devname),
>>>> -			"%s/" PCI_PRI_FMT "/resource%d",
>>>> +			"%s/" PCI_PRI_FMT "/resource%d_wc",
>>>>  			pci_get_sysfs_path(),
>>>>  			loc->domain, loc->bus, loc->devid,
>>>>  			loc->function, res_idx);
>>>> @@ -335,13 +335,22 @@
>>>>  	}
>>>>  
>>>>  	/*
>>>> -	 * open resource file, to mmap it
>>>> +	 * open prefetchable resource file first, try to mmap it
>>>>  	 */
>>>>  	fd = open(devname, O_RDWR);
>>>>  	if (fd < 0) {
>>>> -		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>>>> -				devname, strerror(errno));
>>>> -		goto error;
>>>> +		snprintf(devname, sizeof(devname),
>>>> +				"%s/" PCI_PRI_FMT "/resource%d",
>>>> +				pci_get_sysfs_path(),
>>>> +				loc->domain, loc->bus, loc->devid,
>>>> +				loc->function, res_idx);
>>>> +		/* then try to map resource file */
>>>> +		fd = open(devname, O_RDWR);
>>>> +		if (fd < 0) {
>>>> +			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>>>> +					devname, strerror(errno));
>>>> +			goto error;
>>>> +		}
>>>>  	}
>>>>  
>>>>  	/* try mapping somewhere close to the end of hugepages */
>>>>
>>>
>
Liu, Changpeng Oct. 29, 2018, 12:48 a.m. UTC | #5 
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Sunday, October 28, 2018 9:59 AM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; dpdk-dev
> <dev@dpdk.org>; Rafal Kozik <rk@semihalf.com>
> Subject: Re: [dpdk-dev] [PATCH] pci/uio: enable prefetchable resources mapping
> 
> On 10/5/2017 9:33 AM, bruce.richardson at intel.com (Bruce Richardson) wrote:
> > On Thu, Oct 05, 2017 at 09:28:34AM +0100, Bruce Richardson wrote:
> >> On Thu, Oct 05, 2017 at 01:06:41AM +0100, Ferruh Yigit wrote:
> >>> On 6/3/2017 11:57 PM, Changpeng Liu wrote:
> >>>> For PCI prefetchable resources, Linux will create a
> >>>> write combined file as well, the library will try
> >>>> to map resourceX_wc file first, if the file does
> >>>> not exist, then it will map resourceX as usual.
> >>>
> >>> Hi Changpeng,
> >>>
> >>> Code part looks OK, but can you please describe more why we should try
> >>> write combined resource file first, what is the benefit of using it _wc
> >>> file?
> >>>
> >>> Thanks,
> >>> ferruh
> >>>
> >> Also, if we use a write combining resource file, I believe we will use
> > s/will use/will lose/
> >
> >> correct ordering of instructions within our drivers. Does applying this
> >> patch not also mean that we would need memory barriers inside all the
> >> drivers, so as to ensure that we don't have a queue doorbell write
> >> reordered with the descriptor writes? I don't think it's safe to apply
> >> this change on it's own, without driver changes, since all PMDs assume
> >> strong ordering on IA.
> >>
> >> Regards,
> >> /Bruce
> 
> Hi Changpeng,
> 
> Following patch looks like superseded this patch, can you please check it. I
> will update the status of this patch in patchwork.
> 
> https://patches.dpdk.org/project/dpdk/list/?series=323&state=%2A&archive=bot
> h
> https://patches.dpdk.org/patch/41971/
Yeah, this feature has been enabled with DPDK now, you can update the patch status from me.
> 
> 
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Changpeng Liu <changpeng.liu at intel.com>
> >>>> ---
> >>>>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 19 ++++++++++++++-----
> >>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> >>>> index fa10329..d9fc20a 100644
> >>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> >>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> >>>> @@ -321,7 +321,7 @@
> >>>>
> >>>>  	/* update devname for mmap  */
> >>>>  	snprintf(devname, sizeof(devname),
> >>>> -			"%s/" PCI_PRI_FMT "/resource%d",
> >>>> +			"%s/" PCI_PRI_FMT "/resource%d_wc",
> >>>>  			pci_get_sysfs_path(),
> >>>>  			loc->domain, loc->bus, loc->devid,
> >>>>  			loc->function, res_idx);
> >>>> @@ -335,13 +335,22 @@
> >>>>  	}
> >>>>
> >>>>  	/*
> >>>> -	 * open resource file, to mmap it
> >>>> +	 * open prefetchable resource file first, try to mmap it
> >>>>  	 */
> >>>>  	fd = open(devname, O_RDWR);
> >>>>  	if (fd < 0) {
> >>>> -		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> >>>> -				devname, strerror(errno));
> >>>> -		goto error;
> >>>> +		snprintf(devname, sizeof(devname),
> >>>> +				"%s/" PCI_PRI_FMT "/resource%d",
> >>>> +				pci_get_sysfs_path(),
> >>>> +				loc->domain, loc->bus, loc->devid,
> >>>> +				loc->function, res_idx);
> >>>> +		/* then try to map resource file */
> >>>> +		fd = open(devname, O_RDWR);
> >>>> +		if (fd < 0) {
> >>>> +			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> >>>> +					devname, strerror(errno));
> >>>> +			goto error;
> >>>> +		}
> >>>>  	}
> >>>>
> >>>>  	/* try mapping somewhere close to the end of hugepages */
> >>>>
> >>>
> >
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
index fa10329..d9fc20a 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
@@ -321,7 +321,7 @@ 
 
 	/* update devname for mmap  */
 	snprintf(devname, sizeof(devname),
-			"%s/" PCI_PRI_FMT "/resource%d",
+			"%s/" PCI_PRI_FMT "/resource%d_wc",
 			pci_get_sysfs_path(),
 			loc->domain, loc->bus, loc->devid,
 			loc->function, res_idx);
@@ -335,13 +335,22 @@ 
 	}
 
 	/*
-	 * open resource file, to mmap it
+	 * open prefetchable resource file first, try to mmap it
 	 */
 	fd = open(devname, O_RDWR);
 	if (fd < 0) {
-		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
-				devname, strerror(errno));
-		goto error;
+		snprintf(devname, sizeof(devname),
+				"%s/" PCI_PRI_FMT "/resource%d",
+				pci_get_sysfs_path(),
+				loc->domain, loc->bus, loc->devid,
+				loc->function, res_idx);
+		/* then try to map resource file */
+		fd = open(devname, O_RDWR);
+		if (fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+					devname, strerror(errno));
+			goto error;
+		}
 	}
 
 	/* try mapping somewhere close to the end of hugepages */