From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id C49733DC for ; Wed, 14 Dec 2016 16:15:52 +0100 (CET) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP; 14 Dec 2016 07:15:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,347,1477983600"; d="scan'208";a="202535265" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.64]) by fmsmga004.fm.intel.com with SMTP; 14 Dec 2016 07:15:41 -0800 Received: by (sSMTP sendmail emulation); Wed, 14 Dec 2016 15:15:39 +0000 Date: Wed, 14 Dec 2016 15:15:39 +0000 From: Bruce Richardson To: Jerin Jacob Cc: "Van Haaren, Harry" , "dev@dpdk.org" , "thomas.monjalon@6wind.com" , "hemant.agrawal@nxp.com" , "Eads, Gage" Message-ID: <20161214151539.GA110884@bricha3-MOBL3.ger.corp.intel.com> References: <1479447902-3700-2-git-send-email-jerin.jacob@caviumnetworks.com> <1480996340-29871-1-git-send-email-jerin.jacob@caviumnetworks.com> <1480996340-29871-2-git-send-email-jerin.jacob@caviumnetworks.com> <20161208012413.GA22031@svelivela-lt.caveonetworks.com> <20161214131356.GA4224@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161214131356.GA4224@localhost.localdomain> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.7.1 (2016-10-04) Subject: Re: [dpdk-dev] [PATCH v2 1/6] eventdev: introduce event driven programming model 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: Wed, 14 Dec 2016 15:15:53 -0000 On Wed, Dec 14, 2016 at 06:43:58PM +0530, Jerin Jacob wrote: > On Thu, Dec 08, 2016 at 11:02:16AM +0000, Van Haaren, Harry wrote: > > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com] > > > Sent: Thursday, December 8, 2016 1:24 AM > > > To: Van Haaren, Harry > > > > > > > > > > * Operation and sched_type *increased* to 4 bits each (from previous value of 2) to > > > allow future expansion without ABI changes > > > > > > Anyway it will break ABI if we add new operation. I would propose to keep 4bit > > > reserved and add it when required. > > > > Ok sounds good. I'll suggest to move it to the middle between operation or sched type, which would allow expanding operation without ABI breaks. On expanding the field would remain in the same place with the same bits available in that place (no ABI break), but new bits can be added into the currently reserved space. > > OK. We will move the rsvd field as you suggested. > > > > > > > > > * Restore flow_id to 24 bits of a 32 bit int (previous size was 20 bits) > > > > * sub-event-type reduced to 4 bits (previous value was 8 bits). Can we think of > > > situations where 16 values for application specified identifiers of each event-type is > > > genuinely not enough? > > > One packet will not go beyond 16 stages but an application may have more stages and > > > each packet may go mutually exclusive stages. For example, > > > > > > packet 0: stagex_0 ->stagex_1 > > > packet 1: stagey_0 ->stagey_1 > > > > > > In that sense, IMO, more than 16 is required.(AFIAK, VPP has any much larger limit on > > > number of stages) > > > > My understanding was that stages are linked to event queues, so the application can determine the stage the packet comes from by reading queue_id? > > That is one way of doing it. But it is limited to number of queues > therefore scalability issues.Another approach is through > sub_event_type scheme without depended on the number of queues. > > > > > I'm not opposed to having an 8 bit sub_event_type, but it seems unnecessarily large from my point of view. If you have a use for it, I'm ok with 8 bits. > > OK > > > > > > > > > In my opinion this structure layout is more balanced, and will perform better due to > > > less loads that will need masking to access the required value. > > > OK. Considering more balanced layout and above points. I propose following scheme(based on > > > your input) > > > > > > union { > > > uint64_t event; > > > struct { > > > uint32_t flow_id: 20; > > > uint32_t sub_event_type : 8; > > > uint32_t event_type : 4; > > > > > > uint8_t rsvd: 4; /* for future additions */ > > > uint8_t operation : 2; /* new fwd drop */ > > > uint8_t sched_type : 2; > > > > > > uint8_t queue_id; > > > uint8_t priority; > > > uint8_t impl_opaque; > > > }; > > > }; > > > > > > Feedback and improvements welcomed, > > > > > > So incorporating my latest suggestions on moving fields around, excluding sub_event_type *size* changes: > > > > union { > > uint64_t event; > > struct { > > uint32_t flow_id: 20; > > uint32_t event_type : 4; > > uint32_t sub_event_type : 8; /* 8 bits now naturally aligned */ > > Just one suggestion here. I am not sure about the correct split between > number of bits to represent flow_id and sub_event_type fields. And its > connected in our implementation, so I propose to move sub_event_type up so > that future flow_id/sub_event_type bit field size change request wont > impact our implementation. Since it is represented as 32bit as whole, I > don't think there is an alignment issue. > > So incorporating my latest suggestions on moving sub_event_type field around: > > union { > uint64_t event; > struct { > uint32_t flow_id: 20; > uint32_t sub_event_type : 8; > uint32_t event_type : 4; > The issue with the above layout is that you have an 8-bit value which can never be accessed as a byte. With the layout above proposed by Harry, the sub_event_type can be accessed without any bit manipultaion operations just by doing a byte read. With the layout you propose, all fields require masking and/or shifting to access. It won't affect the scheduler performance for us, but it means potentially more cycles in the app to access those fields. /Bruce