From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 435462B99 for ; 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" To: Yuanhan Liu CC: "dev@dpdk.org" , "Richardson, Bruce" , "Ananyev, Konstantin" , "De Lara Guarch, Pablo" , "thomas@monjalon.net" , "maxime.coquelin@redhat.com" , "mtetsuyah@gmail.com" , "Yigit, Ferruh" 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: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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