patches for DPDK stable branches
 help / color / Atom feed
From: "Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com>
To: Ferruh Yigit <ferruh.yigit@intel.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-stable] [PATCH 1/3] net/enic: restrict several handlers to primary process
Date: Wed, 9 Oct 2019 08:48:15 +0000
Message-ID: <BN7PR11MB27382F471BDCA3B8ABD38495BF950@BN7PR11MB2738.namprd11.prod.outlook.com> (raw)
In-Reply-To: <f4065d15-2d7b-3f6b-cc1f-c1af30a7431a@intel.com>

> -----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)..

Thanks.
-Hyong


  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190906065020.21206-1-hyonkim@cisco.com>
2019-09-06  6:50 ` Hyong Youb Kim
2019-10-09  8:01   ` Ferruh Yigit
2019-10-09  8:48     ` Hyong Youb Kim (hyonkim) [this message]
2019-10-09  9:28       ` Ferruh Yigit
2019-10-09  9:38         ` Hyong Youb Kim (hyonkim)
2019-10-09 17:17           ` Ferruh Yigit
2019-09-06  6:50 ` [dpdk-stable] [PATCH 2/3] net/enic: fix two issues in probe for secondary process Hyong Youb Kim
2019-09-06  6:50 ` [dpdk-stable] [PATCH 3/3] net/enic: fix crash in " Hyong Youb Kim

Reply instructions:

You may reply publically 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=BN7PR11MB27382F471BDCA3B8ABD38495BF950@BN7PR11MB2738.namprd11.prod.outlook.com \
    --to=hyonkim@cisco.com \
    --cc=dev@dpdk.org \
    --cc=dirk.lenz@ng4t.com \
    --cc=ferruh.yigit@intel.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

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/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 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


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