From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <jianfeng.tan@intel.com>
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
by dpdk.org (Postfix) with ESMTP id 435462B99
for <dev@dpdk.org>; Thu, 28 Sep 2017 15:50:24 +0200 (CEST)
Received: from fmsmga005.fm.intel.com ([10.253.24.32])
by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
28 Sep 2017 06:50:23 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.42,450,1500966000"; d="scan'208";a="156572437"
Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204])
by fmsmga005.fm.intel.com with ESMTP; 28 Sep 2017 06:50:23 -0700
Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by
FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS)
id 14.3.319.2; Thu, 28 Sep 2017 06:50:23 -0700
Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by
FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS)
id 14.3.319.2; Thu, 28 Sep 2017 06:50:22 -0700
Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.213]) by
SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002;
Thu, 28 Sep 2017 21:50:21 +0800
From: "Tan, Jianfeng" <jianfeng.tan@intel.com>
To: Yuanhan Liu <yliu@fridaylinux.org>
CC: "dev@dpdk.org" <dev@dpdk.org>, "Richardson, Bruce"
<bruce.richardson@intel.com>, "Ananyev, Konstantin"
<konstantin.ananyev@intel.com>, "De Lara Guarch, Pablo"
<pablo.de.lara.guarch@intel.com>, "thomas@monjalon.net"
<thomas@monjalon.net>, "maxime.coquelin@redhat.com"
<maxime.coquelin@redhat.com>, "mtetsuyah@gmail.com" <mtetsuyah@gmail.com>,
"Yigit, Ferruh" <ferruh.yigit@intel.com>
Thread-Topic: [PATCH 06/12] eal: add channel for primary/secondary
communication
Thread-Index: AQHTHYYQPxKxgHD6mEi1G/LWRVZbYqLIVKiAgAHguqA=
Date: Thu, 28 Sep 2017 13:50:20 +0000
Message-ID: <ED26CBA2FAD1BF48A8719AEF02201E36512F9A52@SHSMSX103.ccr.corp.intel.com>
References: <1503654052-84730-1-git-send-email-jianfeng.tan@intel.com>
<1503654052-84730-7-git-send-email-jianfeng.tan@intel.com>
<20170927121947.GL2251@yliu-home>
In-Reply-To: <20170927121947.GL2251@yliu-home>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.239.127.40]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH 06/12] eal: add channel for primary/secondary
communication
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Thu, 28 Sep 2017 13:50:25 -0000
Yuanhan,
Thank you for the detailed review! Most of your suggestions are very good a=
nd I'll fix them in next version.
> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Wednesday, September 27, 2017 8:20 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Richardson, Bruce; Ananyev, Konstantin; De Lara Guarch,
> Pablo; thomas@monjalon.net; maxime.coquelin@redhat.com;
> mtetsuyah@gmail.com; Yigit, Ferruh
> Subject: Re: [PATCH 06/12] eal: add channel for primary/secondary
> communication
>=20
[...]
> > +int
> > +rte_eal_primary_secondary_add_action(const char *action_name,
> > + rte_eal_primary_secondary_t action)
> > +{
> > + struct action_entry *entry =3D malloc(sizeof(struct action_entry));
> > +
> > + if (entry =3D=3D NULL)
> > + return -ENOMEM;
> > +
> > + strncpy(entry->action_name, action_name,
> MAX_ACTION_NAME_LEN);
> > + entry->action =3D action;
> > + TAILQ_INSERT_TAIL(&action_entry_list, entry, next);
>=20
> Since you intended to support "one primary process and multiple secondary
> process", here we need a lock to protect the list.
Only one thread of each process (either primary or secondary) does the regi=
ster. So I wonder we don't have to add lock? Of course, no harm to add a lo=
ck.
>=20
> Another wonder is do we really need that, I mean 1:N model?
I'm open to suggestions. IMO, not much extra code for 1:N model than 1:1 mo=
del. So not necessary to restrict that.
>=20
> > + return 0;
> > +}
> > +
> > +void
> > +rte_eal_primary_secondary_del_action(const char *name)
> > +{
> > + struct action_entry *entry =3D find_action_entry_by_name(name);
> > +
> > + TAILQ_REMOVE(&action_entry_list, entry, next);
> > + free(entry);
> > +}
> > +
> > +#define MAX_SECONDARY_PROCS 8
> > +
> > +static int efd_pri_sec; /* epoll fd for primary/secondary channel thre=
ad */
>=20
> I think it's not a good idea to use "pri". For me, "private" comes to
> my mind firstly but not "primary".
>=20
> > +static int fd_listen; /* unix listen socket by primary */
> > +static int fd_to_pri; /* only used by secondary process */
> > +static int fds_to_sec[MAX_SECONDARY_PROCS];
>=20
> Too many vars. I'd suggest to use a struct here, which could also make
> the naming a bit simpler. For instance,
>=20
> struct mp_fds {
> int efd;
>=20
> union {
> /* fds for primary process */
> struct {
> int listen;
> /* fds used to send msg to secondary process */
> int secondaries[...];
> };
>=20
> /* fds for secondary process */
> struct {
> /* fds used to send msg to the primary process */
> int primary;
> };
> };
> };
>=20
> It also separates the scope well. Note that above field naming does
> not like perfect though. Feel free to come up with some better names.
You can always make the code look so clean, thank you!
[...]
> > +/** Path of primary/secondary communication unix socket file. */
> > +#define PRIMARY_SECONDARY_UNIX_PATH_FMT "%s/.%s_unix"
> > +static inline const char *
> > +eal_primary_secondary_unix_path(void)
> > +{
> > + static char buffer[PATH_MAX]; /* static so auto-zeroed */
> > + const char *directory =3D default_config_dir;
> > + const char *home_dir =3D getenv("HOME");
>=20
> It's not a good practice to generate such file at HOME dir. User would
> be surprised to find it at HOME dir. In the worst case, user might delete
> it.
This way is the legacy way in DPDK, for example the config path. So I think=
we should fix that in another patch.
>=20
> The more common way is to put it to tmp dir, like "/tmp".
Thanks,
Jianfeng