Re: [dpdk-dev] [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 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