On Thu, Dec 08, 2016 at 11:02:16AM +0000, Van Haaren, Harry wrote:
> > From: Jerin Jacob [mailto:[email protected]]
> > Sent: Thursday, December 8, 2016 1:24 AM
> > To: Van Haaren, Harry <[email protected]>
>
> <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;
uint8_t operation : 2; /* new fwd drop */
uint8_t rsvd: 4; /* for future additions, can be expanded into
without ABI break */
uint8_t sched_type : 2;
uint8_t queue_id;
uint8_t priority;
uint8_t impl_opaque;
};
};
