DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: "Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"John Daley (johndale)" <johndale@cisco.com>,
	Dirk-Holger Lenz <dirk.lenz@ng4t.com>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to primary process
Date: Wed, 9 Oct 2019 18:17:05 +0100	[thread overview]
Message-ID: <68962f8b-1ff0-e098-0f15-bc7e857cc666@intel.com> (raw)
In-Reply-To: <BN7PR11MB273831F4BC4573596669D330BF950@BN7PR11MB2738.namprd11.prod.outlook.com>

On 10/9/2019 10:38 AM, Hyong Youb Kim (hyonkim) wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, October 9, 2019 6:28 PM
>> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
>> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to
>> primary process
>>
>> On 10/9/2019 9:48 AM, Hyong Youb Kim (hyonkim) wrote:
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Sent: Wednesday, October 9, 2019 5:02 PM
>>>> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>>>> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
>>>> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
>>>> Subject: Re: [PATCH 1/3] net/enic: restrict several handlers to primary
>>>> process
>>>>
>>>> On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
>>>>> These eth_dev_ops handlers should run only in the primary process.
>>>>> - filter_ctrl
>>>>> - reta_update
>>>>> - rss_hash_update
>>>>> - set_mc_addr_list
>>>>> - udp_tunnel_port_add
>>>>> - udp_tunnel_port_del
>>>>>
>>>>> Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
>>>>> Fixes: 8d496995346c ("net/enic: support multicast filtering")
>>>>> Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port
>>>> number")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>>>> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
>>>>> Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>>>> Reviewed-by: John Daley <johndale@cisco.com>
>>>>> ---
>>>>>  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/enic/enic_ethdev.c
>>>> b/drivers/net/enic/enic_ethdev.c
>>>>> index 06dc67122..85d785e62 100644
>>>>> --- a/drivers/net/enic/enic_ethdev.c
>>>>> +++ b/drivers/net/enic/enic_ethdev.c
>>>>> @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
>> *dev,
>>>>>  {
>>>>>  	int ret = 0;
>>>>>
>>>>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>>> +		return -E_RTE_SECONDARY;
>>>>>  	ENICPMD_FUNC_TRACE();
>>>>>
>>>>>  	switch (filter_type) {
>>>>
>>>> I remember we have similar talk with John in the past about these
>> secondary
>>>> application checks in ethdev_ops.
>>>>
>>>> I would like to understand why these checks required only in enic, can you
>>>> please describe your use case?
>>>> Is there any reason secondary application can't change configuration of
>> the
>>>> device, or are you updating your driver to work with specific application?
>>>
>>> Hi,
>>>
>>> No fundamental reasons why secondary processes cannot run these
>>> handlers. These checks are to make it clear that it is not safe to do
>>> so at the moment. It is a software limitation.
>>>
>>> The firmware API (devcmd) we use to configure NIC settings assumes one
>>> user executing one command at a time. And, many of the handlers in the
>>> driver also assume primary process. The firmware itself has
>>> enough checks to prevent concurrent devcmd attempts from corrupting
>>> its internal state. But, host processes can get confused. For example,
>>> process A gets process B's results, or overwrites B's devcmd, etc.
>>>
>>> I believe these issues are all fixable in the driver. We could use
>>> locks in shared memory to serialize devcmd (though risky), fix
>>> handlers that assume primary process, and so on. It is a to-do item for
>>> this driver and would require its own patch series (e.g. allow
>>> secondary processes to run X, Y, Z safely)..
>>
>> What you have described is valid concern for all drivers, that synchronization
>> has been pushed to the application level.
>>
>> I don't see the point of just putting protection to only one driver.
>>
>> And as a alternative, what do you think about having a check in the prob for
>> the
>> secondary process and assign a subset of the ethdev_ops in that case? This
>> makes
>>  more clear what is supported in the secondary process, and it prevents
>> putting
>> secondary process checks everywhere.
> 
> Hi,
> 
> Okay, that sounds reasonable. Could you drop this one patch and apply
> the rest in the series? I may not have time to properly re-do this one
> in this cycle..

OK, will mark 1/3 as "Change Requested" and continue with others. Thanks.

  reply	other threads:[~2019-10-09 17:17 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-06  6:50 [dpdk-dev] [PATCH 0/3] net/enic: fix multi-process isseus Hyong Youb Kim
2019-09-06  6:50 ` [dpdk-dev] [PATCH 1/3] net/enic: restrict several handlers to primary process Hyong Youb Kim
2019-10-09  8:01   ` Ferruh Yigit
2019-10-09  8:48     ` Hyong Youb Kim (hyonkim)
2019-10-09  9:28       ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2019-10-09  9:38         ` Hyong Youb Kim (hyonkim)
2019-10-09 17:17           ` Ferruh Yigit [this message]
2019-09-06  6:50 ` [dpdk-dev] [PATCH 2/3] net/enic: fix two issues in probe for secondary process Hyong Youb Kim
2019-09-06  6:50 ` [dpdk-dev] [PATCH 3/3] net/enic: fix crash in " Hyong Youb Kim
2019-10-10 11:39 ` [dpdk-dev] [PATCH 0/3] net/enic: fix multi-process isseus 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=68962f8b-1ff0-e098-0f15-bc7e857cc666@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=dev@dpdk.org \
    --cc=dirk.lenz@ng4t.com \
    --cc=hyonkim@cisco.com \
    --cc=johndale@cisco.com \
    --cc=stable@dpdk.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).