diff mbox

[dpdk-dev,2/6] eal: Close file descriptor of uio configuration

Message ID 1426584645-28828-3-git-send-email-mukawa@igel.co.jp (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Tetsuya Mukawa March 17, 2015, 9:30 a.m. UTC 
  When pci_uio_unmap_resource() is called, a file descriptor that is used
for uio configuration should be closed.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

David Marchand March 18, 2015, 3:19 p.m. UTC | #1 
On Tue, Mar 17, 2015 at 10:30 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:

> When pci_uio_unmap_resource() is called, a file descriptor that is used
> for uio configuration should be closed.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index 9cdf24f..b971ec9 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -459,8 +459,14 @@ pci_uio_unmap_resource(struct rte_pci_device *dev)
>
>         /* close fd if in primary process */
>         close(dev->intr_handle.fd);
> -
>         dev->intr_handle.fd = -1;
> +
> +       /* close cfg_fd if in primary process */

+       if (dev->intr_handle.uio_cfg_fd >= 0) {
> +               close(dev->intr_handle.uio_cfg_fd);
> +               dev->intr_handle.uio_cfg_fd = -1;
> +       }
> +
>         dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
>  }
>  #endif /* RTE_LIBRTE_EAL_HOTPLUG */
>

Hum, why check for < 0 value ?
There is no such check for intr_handle.fd and I can see no reason why we
should behave differently.
Iremonger, Bernard March 19, 2015, 4:04 p.m. UTC | #2 
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Tuesday, March 17, 2015 9:31 AM
> To: dev@dpdk.org
> Cc: Iremonger, Bernard; Richardson, Bruce; Tetsuya Mukawa
> Subject: [PATCH 2/6] eal: Close file descriptor of uio configuration
> 
> When pci_uio_unmap_resource() is called, a file descriptor that is used for uio configuration should be
> closed.
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index 9cdf24f..b971ec9 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -459,8 +459,14 @@ pci_uio_unmap_resource(struct rte_pci_device *dev)
> 
>  	/* close fd if in primary process */
Hi Tetsuya,

Should there be a check for the primary process before closing both of the these files?

Regards,

Bernard.

>  	close(dev->intr_handle.fd);
> -
>  	dev->intr_handle.fd = -1;
> +
> +	/* close cfg_fd if in primary process */
> +	if (dev->intr_handle.uio_cfg_fd >= 0) {
> +		close(dev->intr_handle.uio_cfg_fd);
> +		dev->intr_handle.uio_cfg_fd = -1;
> +	}
> +
>  	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;  }  #endif /*
> RTE_LIBRTE_EAL_HOTPLUG */
> --
> 1.9.1
Tetsuya Mukawa March 20, 2015, 1:53 a.m. UTC | #3 
On 2015/03/19 0:19, David Marchand wrote:
> On Tue, Mar 17, 2015 at 10:30 AM, Tetsuya Mukawa <mukawa@igel.co.jp
> <mailto:mukawa@igel.co.jp>> wrote:
>
>     When pci_uio_unmap_resource() is called, a file descriptor that is
>     used
>     for uio configuration should be closed.
>
>     Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp
>     <mailto:mukawa@igel.co.jp>>
>     ---
>      lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 8 +++++++-
>      1 file changed, 7 insertions(+), 1 deletion(-)
>
>     diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     index 9cdf24f..b971ec9 100644
>     --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     @@ -459,8 +459,14 @@ pci_uio_unmap_resource(struct rte_pci_device
>     *dev)
>
>             /* close fd if in primary process */
>             close(dev->intr_handle.fd);
>     -
>             dev->intr_handle.fd = -1;
>     +
>     +       /* close cfg_fd if in primary process */
>
>     +       if (dev->intr_handle.uio_cfg_fd >= 0) {
>     +               close(dev->intr_handle.uio_cfg_fd);
>     +               dev->intr_handle.uio_cfg_fd = -1;
>     +       }
>     +
>             dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
>      }
>      #endif /* RTE_LIBRTE_EAL_HOTPLUG */
>
>
> Hum, why check for < 0 value ?
> There is no such check for intr_handle.fd and I can see no reason why
> we should behave differently.
>

Hi David,

There is a reason, but 'if-condition' should move to later patch.

When we move this function to eal common, bsdapp will not use cfg_fd at all.
(Nic uio of BSD will not use this file descriptor.)
So I added the 'if-condition' like above.
But, I will move this fixing in later patch that actually consolidate
this function.

Thanks,
Tetsuya


>
> -- 
> David Marchand
Tetsuya Mukawa March 20, 2015, 1:54 a.m. UTC | #4 
On 2015/03/20 1:04, Iremonger, Bernard wrote:
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>> Sent: Tuesday, March 17, 2015 9:31 AM
>> To: dev@dpdk.org
>> Cc: Iremonger, Bernard; Richardson, Bruce; Tetsuya Mukawa
>> Subject: [PATCH 2/6] eal: Close file descriptor of uio configuration
>>
>> When pci_uio_unmap_resource() is called, a file descriptor that is used for uio configuration should be
>> closed.
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>> ---
>>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>> index 9cdf24f..b971ec9 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>> @@ -459,8 +459,14 @@ pci_uio_unmap_resource(struct rte_pci_device *dev)
>>
>>  	/* close fd if in primary process */
> Hi Tetsuya,
>
> Should there be a check for the primary process before closing both of the these files?

Hi Bernard,

Yes, the check is done like below.
(But it doesn't appear in this patch.)

void
pci_uio_unmap_resource(struct rte_pci_device *dev)
{
        struct mapped_pci_resource *uio_res;
        struct mapped_pci_res_list *uio_res_list =
                        RTE_TAILQ_CAST(rte_uio_tailq.head,
mapped_pci_res_list);

        if (dev == NULL)
                return;

        /* find an entry for the device */
        uio_res = pci_uio_find_resource(dev);
        if (uio_res == NULL)
                return;

        /* secondary processes - just free maps */
        if (rte_eal_process_type() != RTE_PROC_PRIMARY)
                return pci_uio_unmap(uio_res);

        TAILQ_REMOVE(uio_res_list, uio_res, next);

        (snip)

        /* close fd if in primary process */
        close(dev->intr_handle.fd);
        dev->intr_handle.fd = -1;

        (snip)
}

Regards,
Tetsuya

> Regards,
>
> Bernard.
>
>>  	close(dev->intr_handle.fd);
>> -
>>  	dev->intr_handle.fd = -1;
>> +
>> +	/* close cfg_fd if in primary process */
>> +	if (dev->intr_handle.uio_cfg_fd >= 0) {
>> +		close(dev->intr_handle.uio_cfg_fd);
>> +		dev->intr_handle.uio_cfg_fd = -1;
>> +	}
>> +
>>  	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;  }  #endif /*
>> RTE_LIBRTE_EAL_HOTPLUG */
>> --
>> 1.9.1
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 9cdf24f..b971ec9 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
@@ -459,8 +459,14 @@  pci_uio_unmap_resource(struct rte_pci_device *dev)
 
 	/* close fd if in primary process */
 	close(dev->intr_handle.fd);
-
 	dev->intr_handle.fd = -1;
+
+	/* close cfg_fd if in primary process */
+	if (dev->intr_handle.uio_cfg_fd >= 0) {
+		close(dev->intr_handle.uio_cfg_fd);
+		dev->intr_handle.uio_cfg_fd = -1;
+	}
+
 	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 }
 #endif /* RTE_LIBRTE_EAL_HOTPLUG */