On 9/19/2017 6:31 AM, John Daley (johndale) wrote: > >> -----Original Message----- >> From: Thomas Monjalon [mailto:tho...@monjalon.net] >> Sent: Monday, September 18, 2017 3:25 PM >> To: John Daley (johndale) <johnd...@cisco.com> >> Cc: Ferruh Yigit <ferruh.yi...@intel.com>; dev@dpdk.org; Sergio Gonzalez >> Monroy <sergio.gonzalez.mon...@intel.com> >> Subject: Re: [PATCH] net/enic: fix multi-process operation >> >> 18/09/2017 23:27, Ferruh Yigit: >>> On 9/11/2017 7:58 PM, John Daley wrote: >>>> - Use rte_malloc() instead of malloc() for the per device 'vdev' structure >>>> so that it can be shared across processes. >>>> - Only initialize the device if the process type is RTE_PROC_PRIMARY >>>> - Only allow the primary process to do queue setup, start/stop, promisc >>>> allmulticast, mac add/del, mtu. >> [...] >>>> --- a/drivers/net/enic/enic_ethdev.c >>>> +++ b/drivers/net/enic/enic_ethdev.c >>>> @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev >>>> *dev, static void enicpmd_dev_tx_queue_release(void *txq) { >>>> ENICPMD_FUNC_TRACE(); >>>> + >>>> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) >>>> + return; >>>> + >>> >>> Hi John, >>> >>> I am not sure about these updates. Agree that these functions should >>> know process type, but all others PMDs don't do this. > > I looked at mlx5 and it does something similar with its mlx5_is_secondary() > in functions that modify fields in its shared private structure.
Right, mlx5 also has these checks. > >>> >>> Added a few more people for comment, but as far I understand its >>> application responsibility to NOT call these functions if it is >>> secondary process. >>> >>> For device init/uninit, that is part of eal_init() and have to be >>> called both for primary and secondary process and PMD needs to protect >>> it, for other functions application's responsibility. > > I put the checks into the PMD because I didn't want to trust the app and I > didn't see checks in the library functions. I assumed that is why it was done > in mlx5. I was afraid that the secondary may try to write fields in the > shared structure and cause some silent bad behavior, like if secondary sets > the MTU then the primary does, then secondary assumes it was different than > what it is, or something like that. The set values are in the shared memory, so a variable set by secondary will be visible to primary, via versa. Of course a synchronization required between primary and secondary processes. Also why secondary shouldn't be allowed to do some work, like start/stop the port? I believe this should be application level concern, at worst libraries but not drivers. >> >> Yes for now it is the policy. >> But it is a gray area and it could be clearer with my "ownership proposal": >> http://dpdk.org/ml/archives/dev/2017-September/074656.html >> A secondary process could manage the ports it owns. > > I think the ownership concept would help make DPDK more flexible and > potentially cleaner. Perhaps ownership checks could be pushed the lib > functions, like rte_eth_dev_set_mtu(), and remove all such checks in the > PMD(s). >> >> Feel free to comment the proposal.
