Re: [dpdk-dev] [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to primary process

["Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com>]

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

Thanks!
-Hyong