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.ja...@caviumnetworks.com] > > > Sent: Thursday, December 8, 2016 1:24 AM > > > To: Van Haaren, Harry <harry.van.haa...@intel.com> > > > > <snip> > > > > > > * 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
