From: "John Daley (johndale)" <johndale@cisco.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
Thomas Monjalon <thomas@monjalon.net>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>,
Adrien Mazarguil <adrien.mazarguil@6wind.com>,
Nelio Laranjeiro <nelio.laranjeiro@6wind.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Subject: Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
Date: Fri, 22 Sep 2017 13:02:15 +0000 [thread overview]
Message-ID: <d914368ce43446ac90efc106b297a344@XCH-RCD-007.cisco.com> (raw)
In-Reply-To: <a6303f59-ff69-82e8-24e1-abf5c66f3bae@intel.com>
Hi Ferruh,
-johnd
> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, September 19, 2017 5:59 AM
> To: John Daley (johndale) <johndale@cisco.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: dev@dpdk.org; Sergio Gonzalez Monroy
> <sergio.gonzalez.monroy@intel.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; Nelio Laranjeiro
> <nelio.laranjeiro@6wind.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
>
> On 9/19/2017 6:31 AM, John Daley (johndale) wrote:
> >
> >> -----Original Message-----
> >> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >> Sent: Monday, September 18, 2017 3:25 PM
> >> To: John Daley (johndale) <johndale@cisco.com>
> >> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Sergio
> >> Gonzalez Monroy <sergio.gonzalez.monroy@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.
I don't disagree, but I would need to verify that relaxing the checks would not cause catastrophic errors in case the primary and secondary don't synchronize their actions to the PMD. This will take some more testing. The patch I provided with the checks is safe as is and so if you could accept it for 17.08 with the promise that I will work on relaxing the checks in the next release, I would appreciate it.
>
> >>
> >> 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.
next prev parent reply other threads:[~2017-09-22 13:02 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-11 18:58 John Daley
2017-09-12 13:53 ` Aaron Conole
2017-09-18 21:27 ` Ferruh Yigit
2017-09-18 22:25 ` Thomas Monjalon
2017-09-19 5:31 ` John Daley (johndale)
2017-09-19 12:58 ` Ferruh Yigit
2017-09-22 13:02 ` John Daley (johndale) [this message]
2017-09-22 16:50 ` 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=d914368ce43446ac90efc106b297a344@XCH-RCD-007.cisco.com \
--to=johndale@cisco.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=konstantin.ananyev@intel.com \
--cc=nelio.laranjeiro@6wind.com \
--cc=sergio.gonzalez.monroy@intel.com \
--cc=thomas@monjalon.net \
/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).