DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Huisong Li <lihuisong@huawei.com>, Thomas Monjalon <thomas@monjalon.net>
Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
	"dev@dpdk.org" <dev@dpdk.org>,
	Anatoly Burakov <anatoly.burakov@intel.com>,
	David Marchand <david.marchand@redhat.com>
Subject: Re: [dpdk-dev] [RFC V2] ethdev: fix issue that dev close in PMD calls twice
Date: Tue, 24 Aug 2021 15:42:06 +0100
Message-ID: <e36a5ee0-9569-953c-4e54-2880bae82a97@intel.com> (raw)
In-Reply-To: <c57d6f62-4c5f-43b5-728d-669234de51d2@huawei.com>

On 8/19/2021 4:45 AM, Huisong Li wrote:
> 在 2021/8/18 19:24, Ferruh Yigit 写道:
>> On 8/13/2021 9:16 AM, Huisong Li wrote:
>>> 在 2021/8/13 14:12, Thomas Monjalon 写道:
>>>> 13/08/2021 04:11, Huisong Li:
>>>>> Hi, all
>>>>>
>>>>> This patch can enhance the security of device uninstallation to
>>>>> eliminate dependency on user usage methods.
>>>>>
>>>>> Can you check this patch?
>>>>>
>>>>>
>>>>> 在 2021/8/3 10:30, Huisong Li 写道:
>>>>>> Ethernet devices in DPDK can be released by rte_eth_dev_close() and
>>>>>> rte_dev_remove(). These APIs both call xxx_dev_close() in PMD layer
>>>>>> to uninstall hardware. However, the two APIs do not have explicit
>>>>>> invocation restrictions. In other words, at the ethdev layer, it is
>>>>>> possible to call rte_eth_dev_close() before calling rte_dev_remove()
>>>>>> or rte_eal_hotplug_remove(). In such a bad scenario,
>>>> It is not a bad scenario.
>>>> If there is no more port for the device after calling close,
>>>> the device should be removed automatically.
>>>> Keep in mind "close" is for one port, "remove" is for the entire device
>>>> which can have more than one port.
>>> I know.
>>>
>>> dev_close() is for removing an eth device. And rte_dev_remove() can be used
>>>
>>> for removing the rte device and all its eth devices belonging to the rte device.
>>>
>>> In rte_dev_remove(), "remove" is executed in primary or one of secondary,
>>>
>>> all eth devices having same pci address will be closed and removed.
>>>
>>>>>> the primary
>>>>>> process may be fine, but it may cause that xxx_dev_close() in the PMD
>>>>>> layer will be called twice in the secondary process. So this patch
>>>>>> fixes it.
>>>> If a port is closed in primary, it should be the same in secondary.
>>>>
>>>>
>>>>>> +    /*
>>>>>> +     * The eth_dev->data->name doesn't be cleared by the secondary process,
>>>>>> +     * so above "eth_dev" isn't NULL after rte_eth_dev_close() called.
>>>> This assumption is not clear. All should be closed together.
>>> However, dev_close() does not have the feature similar to rte_dev_remove().
>>>
>>> Namely, it is not guaranteed that all eth devices are closed together in ethdev
>>> layer. It depends on app or user.
>>>
>>> If the app does not close together, the operation of repeatedly uninstalling an
>>> eth device in the secondary process
>>>
>>> will be triggered when dev_close() is first called by one secondary process, and
>>> then rte_dev_remove() is called.
>>>
>>> So I think it should be avoided.
>> First of all, I am not sure about calling 'rte_eth_dev_close()' or
>> 'rte_dev_remove()' from the secondary process.
>> There are explicit checks in various locations to prevent clearing resources
>> completely from secondary process.
> 
> There's no denying that.
> 
> Generally, hardware resources of eth device and shared data of the primary and
> secondary process
> 
> are cleared by primary, which are controled by ethdev layer or PMD layer.
> 
> But there may be some private data or resources of each process (primary or
> secondary ), such as mp action
> 
> registered by rte_mp_action_register() or others.  For these resources, the
> secondary process still needs to clear.
> 
> Namely, both primary and secondary processes need to prevent repeated offloading
> of resources.
> 
>>
>> Calling 'rte_eth_dev_close()' or 'rte_dev_remove()' by secondary is technically
>> can be done but application needs to be extra cautious and should take extra
>> measures and synchronization to make it work.
>> Regular use-case is secondary processes do the packet processing and all control
>> commands run by primary.
> 
> You are right. We have a consensus that 'rte_eth_dev_close()' or 'rte_dev_remove()'
> 
> can be called by primary and secondary processes.
> 
> But DPDK framework cannot assume user behavior.😁
> 
> We need to make it more secure and reliable for both primary and secondary
> processes.
> 
>>
>> In primary, if you call 'rte_eth_dev_close()' it will clear all ethdev resources
>> and further 'rte_dev_remove()' call will detect missing ethdev resources and
>> won't try to clear them again.
>>
>> In secondary, if you call 'rte_eth_dev_close()', it WON'T clear all resources
>> and further 'rte_dev_remove()' call (either from primary or secondary) will try
>> to clean ethdev resources again. You are trying to prevent this retry in remove
>> happening for secondary process.
> 
> Right. However, if secondary process in PMD layer has its own private resources
> to be
> 
> cleared, it still need to do it by calling 'rte_eth_dev_close()' or
> 'rte_dev_remove()'.
> 
>> In secondary it won't free ethdev resources anyway if you let it continue, but I
>> guess here you are trying to prevent the PMD dev_close() called again. Why? Is
>> it just for optimization or does it cause unexpected behavior in the PMD?
>>
>>
>> Overall, to free resources you need to do the 'rte_eth_dev_close()' or
>> 'rte_dev_remove()' in the primary anyway. So instead of this workaround, I would
>> suggest making PMD dev_close() safe to be called multiple times (if this is the
>> problem.)
> 
> In conclusion,  primary and secondary processes in PMD layer may have their own
> 
> private data and resources, which need to be processed and released.
> 
> Currently,  these for PMD are either handled and cleaned up in dev_close() or
> remove().
> 
> However, code streams in rte_dev_remove() cannot ensure that the uninstallation
> 
> from secondary process will not be repeated if rte_eth_dev_close() is first
> called by
> 
> secondary(primary is ok, plese review this patch).
> 
> I think, this is the same for each PMD and is better suited to doing it in
> ethdev layer.
> 

This patch prevents to call dev_close() twice in the secondary process, is this
fixing a theoretical problem or an actual problem?

If it is an actual problem can you please provide details, callstack of the
problematic case?

>>
>> And again, please re-consider calling 'rte_eth_dev_close()' or
>> 'rte_dev_remove()' from the secondary process.
>>
>>>>>> +     * Namely, whether "eth_dev" is NULL cannot be used to determine whether
>>>>>> +     * an ethdev port has been released.
>>>>>> +     * For both primary process and secondary process, eth_dev->state is
>>>>>> +     * RTE_ETH_DEV_UNUSED, which means the ethdev port has been released.
>>>>>> +     */
>>>>>> +    if (eth_dev->state == RTE_ETH_DEV_UNUSED) {
>>>>>> +        RTE_ETHDEV_LOG(INFO, "The ethdev port has been released.");
>>>>>> +        return 0;
>>>>>> +    }
>>>>
>>>> .
>> .


  reply	other threads:[~2021-08-24 14:42 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-02 12:46 [dpdk-dev] [RFC V1] ethdev: fix the issue that dev uninit may be called twice Huisong Li
2021-08-03  2:30 ` [dpdk-dev] [RFC V2] ethdev: fix issue that dev close in PMD calls twice Huisong Li
2021-08-13  2:11   ` Huisong Li
2021-08-13  6:12     ` Thomas Monjalon
2021-08-13  8:16       ` Huisong Li
2021-08-18 11:24         ` Ferruh Yigit
2021-08-19  3:45           ` Huisong Li
2021-08-24 14:42             ` Ferruh Yigit [this message]
2021-08-25  9:53               ` Huisong Li
2021-09-04  1:23                 ` Huisong Li
2021-09-18  3:31                 ` Huisong Li
2021-09-20 14:07                 ` Ferruh Yigit
2021-09-22  3:31                   ` Huisong Li
2021-09-28  7:19                     ` Singh, Aman Deep
2021-09-30 10:54                       ` Huisong Li
2021-09-30 11:01                         ` Ferruh Yigit
2021-10-08  6:13                           ` lihuisong (C)
2021-08-18  9:47 ` [dpdk-dev] [RFC V1] ethdev: fix the issue that dev uninit may be called twice Singh, Aman Deep
2021-08-24  2:10   ` Huisong Li
2021-10-08  8:21 ` [dpdk-dev] [PATCH] ethdev: fix eth device released repeatedly Huisong Li
2021-10-08 10:23   ` Thomas Monjalon
2021-10-09  1:29     ` lihuisong (C)
2021-10-12 11:39 ` [dpdk-dev] [PATCH V2] " Huisong Li
2021-10-12 15:33   ` Thomas Monjalon
2021-10-14  3:50     ` lihuisong (C)
2021-10-14 12:32     ` lihuisong (C)
2021-10-14 12:50       ` Thomas Monjalon
2021-10-15  3:03         ` lihuisong (C)
2021-10-15  3:44 ` [dpdk-dev] [PATCH V3] " Huisong Li
2021-10-19 13:09   ` Ferruh Yigit
2021-10-21  2:31     ` lihuisong (C)
2021-10-21  2:24 ` [dpdk-dev] [PATCH V4] " Huisong Li
2021-10-21 21:19   ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e36a5ee0-9569-953c-4e54-2880bae82a97@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=anatoly.burakov@intel.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=lihuisong@huawei.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git