diff mbox series

[API-NEXT,1/2] linux-generic: events subtype implementation

Message ID 20170614173527.17118-2-dmitry.ereminsolenikov@linaro.org
State New
Headers show
Series event subtype implementation | expand

Commit Message

Dmitry Eremin-Solenikov June 14, 2017, 5:35 p.m. UTC 
Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

---
 platform/linux-generic/include/odp_buffer_inlines.h  |  2 ++
 platform/linux-generic/include/odp_buffer_internal.h |  3 +++
 platform/linux-generic/odp_event.c                   |  5 +++++
 platform/linux-generic/odp_packet.c                  |  1 +
 platform/linux-generic/odp_pool.c                    | 11 +++++++++++
 5 files changed, 22 insertions(+)

-- 
2.11.0

Comments

Savolainen, Petri (Nokia - FI/Espoo) June 15, 2017, 11:21 a.m. UTC | #1 
> index d71f4464af48..3cd4a73cbefb 100644

> --- a/platform/linux-generic/odp_event.c

> +++ b/platform/linux-generic/odp_event.c

> @@ -19,6 +19,11 @@ odp_event_type_t odp_event_type(odp_event_t event)

>  	return _odp_buffer_event_type(odp_buffer_from_event(event));

>  }

> 

> +odp_event_subtype_t odp_event_subtype(odp_event_t event)

> +{

> +	return _odp_buffer_event_subtype(odp_buffer_from_event(event));

> +}

> +

>  void odp_event_free(odp_event_t event)

>  {

>  	switch (odp_event_type(event)) {

> diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-

> generic/odp_packet.c

> index eb66af2d3b9c..3789feca45f9 100644

> --- a/platform/linux-generic/odp_packet.c

> +++ b/platform/linux-generic/odp_packet.c

> @@ -268,6 +268,7 @@ static inline void packet_init(odp_packet_hdr_t

> *pkt_hdr, uint32_t len)

>  			     CONFIG_PACKET_TAILROOM;

> 

>  	pkt_hdr->input = ODP_PKTIO_INVALID;

> +	pkt_hdr->buf_hdr.event_subtype = ODP_EVENT_PACKET_BASIC;



This is not needed if you update crypto.c with _odp_buffer_event_subtype_set() calls, where _odp_buffer_event_type_set() is done already -right?  Packet_init() is done for every alloc and should avoid setting constant data.

There's also odp_event_types() API. Could you implement that also for completeness.

odp_event_type_t odp_event_types(odp_event_t event, odp_event_subtype_t *subtype)


-Petri
Dmitry Eremin-Solenikov June 16, 2017, 9:03 a.m. UTC | #2 
On 15.06.2017 14:21, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> 

>> index d71f4464af48..3cd4a73cbefb 100644

>> --- a/platform/linux-generic/odp_event.c

>> +++ b/platform/linux-generic/odp_event.c

>> @@ -19,6 +19,11 @@ odp_event_type_t odp_event_type(odp_event_t event)

>>  	return _odp_buffer_event_type(odp_buffer_from_event(event));

>>  }

>>

>> +odp_event_subtype_t odp_event_subtype(odp_event_t event)

>> +{

>> +	return _odp_buffer_event_subtype(odp_buffer_from_event(event));

>> +}

>> +

>>  void odp_event_free(odp_event_t event)

>>  {

>>  	switch (odp_event_type(event)) {

>> diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-

>> generic/odp_packet.c

>> index eb66af2d3b9c..3789feca45f9 100644

>> --- a/platform/linux-generic/odp_packet.c

>> +++ b/platform/linux-generic/odp_packet.c

>> @@ -268,6 +268,7 @@ static inline void packet_init(odp_packet_hdr_t

>> *pkt_hdr, uint32_t len)

>>  			     CONFIG_PACKET_TAILROOM;

>>

>>  	pkt_hdr->input = ODP_PKTIO_INVALID;

>> +	pkt_hdr->buf_hdr.event_subtype = ODP_EVENT_PACKET_BASIC;

> 

> 

> This is not needed if you update crypto.c with _odp_buffer_event_subtype_set() calls, where _odp_buffer_event_type_set() is done already -right?  Packet_init() is done for every alloc and should avoid setting constant data.


I gave this idea a thought. I will update crypto.c (thanks for the
point!), but I still insist that packet_init should set subtype.
Otherwise subtype resetting should go into packet free code (which is
uglier) in my opinion. Consider ODP application receiving IPsec packets
from queue then freeing them for some reason before doing
odp_ipsec_result() call. Packet will be freed, but event_subtype will be
left as PACKET_IPSEC.

> 

> There's also odp_event_types() API. Could you implement that also for completeness.

> 

> odp_event_type_t odp_event_types(odp_event_t event, odp_event_subtype_t *subtype)


Sure, I will do.

-- 
With best wishes
Dmitry
Savolainen, Petri (Nokia - FI/Espoo) June 16, 2017, 9:37 a.m. UTC | #3 
> >>  	switch (odp_event_type(event)) {

> >> diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-

> >> generic/odp_packet.c

> >> index eb66af2d3b9c..3789feca45f9 100644

> >> --- a/platform/linux-generic/odp_packet.c

> >> +++ b/platform/linux-generic/odp_packet.c

> >> @@ -268,6 +268,7 @@ static inline void packet_init(odp_packet_hdr_t

> >> *pkt_hdr, uint32_t len)

> >>  			     CONFIG_PACKET_TAILROOM;

> >>

> >>  	pkt_hdr->input = ODP_PKTIO_INVALID;

> >> +	pkt_hdr->buf_hdr.event_subtype = ODP_EVENT_PACKET_BASIC;

> >

> >

> > This is not needed if you update crypto.c with

> _odp_buffer_event_subtype_set() calls, where _odp_buffer_event_type_set()

> is done already -right?  Packet_init() is done for every alloc and should

> avoid setting constant data.

> 

> I gave this idea a thought. I will update crypto.c (thanks for the

> point!), but I still insist that packet_init should set subtype.

> Otherwise subtype resetting should go into packet free code (which is

> uglier) in my opinion. Consider ODP application receiving IPsec packets

> from queue then freeing them for some reason before doing

> odp_ipsec_result() call. Packet will be freed, but event_subtype will be

> left as PACKET_IPSEC.


OK. We will optimize it later if needed: either set subtype to basic only when it's not already basic, or add subtype as packet_init() argument (to avoid first setting it to basic and then to ipsec).

-Petri
Dmitry Eremin-Solenikov June 16, 2017, 10:21 a.m. UTC | #4 
On 16.06.2017 12:37, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>>  	switch (odp_event_type(event)) {

>>>> diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-

>>>> generic/odp_packet.c

>>>> index eb66af2d3b9c..3789feca45f9 100644

>>>> --- a/platform/linux-generic/odp_packet.c

>>>> +++ b/platform/linux-generic/odp_packet.c

>>>> @@ -268,6 +268,7 @@ static inline void packet_init(odp_packet_hdr_t

>>>> *pkt_hdr, uint32_t len)

>>>>  			     CONFIG_PACKET_TAILROOM;

>>>>

>>>>  	pkt_hdr->input = ODP_PKTIO_INVALID;

>>>> +	pkt_hdr->buf_hdr.event_subtype = ODP_EVENT_PACKET_BASIC;

>>>

>>>

>>> This is not needed if you update crypto.c with

>> _odp_buffer_event_subtype_set() calls, where _odp_buffer_event_type_set()

>> is done already -right?  Packet_init() is done for every alloc and should

>> avoid setting constant data.

>>

>> I gave this idea a thought. I will update crypto.c (thanks for the

>> point!), but I still insist that packet_init should set subtype.

>> Otherwise subtype resetting should go into packet free code (which is

>> uglier) in my opinion. Consider ODP application receiving IPsec packets

>> from queue then freeing them for some reason before doing

>> odp_ipsec_result() call. Packet will be freed, but event_subtype will be

>> left as PACKET_IPSEC.

> 

> OK. We will optimize it later if needed: either set subtype to basic only when it's not already basic, or add subtype as packet_init() argument (to avoid first setting it to basic and then to ipsec).


Agreed.

-- 
With best wishes
Dmitry
Bill Fischofer June 16, 2017, 12:08 p.m. UTC | #5 
On Fri, Jun 16, 2017 at 5:21 AM, Dmitry Eremin-Solenikov
<dmitry.ereminsolenikov@linaro.org> wrote:
> On 16.06.2017 12:37, Savolainen, Petri (Nokia - FI/Espoo) wrote:

>>>>>    switch (odp_event_type(event)) {

>>>>> diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-

>>>>> generic/odp_packet.c

>>>>> index eb66af2d3b9c..3789feca45f9 100644

>>>>> --- a/platform/linux-generic/odp_packet.c

>>>>> +++ b/platform/linux-generic/odp_packet.c

>>>>> @@ -268,6 +268,7 @@ static inline void packet_init(odp_packet_hdr_t

>>>>> *pkt_hdr, uint32_t len)

>>>>>                         CONFIG_PACKET_TAILROOM;

>>>>>

>>>>>    pkt_hdr->input = ODP_PKTIO_INVALID;

>>>>> +  pkt_hdr->buf_hdr.event_subtype = ODP_EVENT_PACKET_BASIC;

>>>>

>>>>

>>>> This is not needed if you update crypto.c with

>>> _odp_buffer_event_subtype_set() calls, where _odp_buffer_event_type_set()

>>> is done already -right?  Packet_init() is done for every alloc and should

>>> avoid setting constant data.

>>>

>>> I gave this idea a thought. I will update crypto.c (thanks for the

>>> point!), but I still insist that packet_init should set subtype.

>>> Otherwise subtype resetting should go into packet free code (which is

>>> uglier) in my opinion. Consider ODP application receiving IPsec packets

>>> from queue then freeing them for some reason before doing

>>> odp_ipsec_result() call. Packet will be freed, but event_subtype will be

>>> left as PACKET_IPSEC.

>>

>> OK. We will optimize it later if needed: either set subtype to basic only when it's not already basic, or add subtype as packet_init() argument (to avoid first setting it to basic and then to ipsec).

>

> Agreed.


packet_init() is a critical-path routine that should not be added to
lightly. Subtypes can be added at zero cost by simply taking a couple
of bits from the parser input flags _odp_packet_input_flags_t in
include/odp/api/plat/packet_types.h and using them to encode packet
subtype. Since these flags are set to zeros anyway by packet_init()
there's no additional cost and zeros will be the value for
ODP_PACKET_BASIC. Note that this will also automatically reset the
subtype on odp_packet_reset() calls, which is something else that's
needed.

odp_event_subtype() will return ODP_EVENT_SUBTYPE_NONE for anything
other than packets anyway, so there's no need to have a subtype
encoding in anything other than packets for now.

>

> --

> With best wishes

> Dmitry
diff mbox series

Patch

diff --git a/platform/linux-generic/include/odp_buffer_inlines.h b/platform/linux-generic/include/odp_buffer_inlines.h
index cf817d907ab5..4c0e73390948 100644
--- a/platform/linux-generic/include/odp_buffer_inlines.h
+++ b/platform/linux-generic/include/odp_buffer_inlines.h
@@ -21,6 +21,8 @@  extern "C" {
 
 odp_event_type_t _odp_buffer_event_type(odp_buffer_t buf);
 void _odp_buffer_event_type_set(odp_buffer_t buf, int ev);
+odp_event_subtype_t _odp_buffer_event_subtype(odp_buffer_t buf);
+void _odp_buffer_event_subtype_set(odp_buffer_t buf, int ev);
 int odp_buffer_snprint(char *str, uint32_t n, odp_buffer_t buf);
 
 static inline odp_buffer_t odp_hdr_to_buf(odp_buffer_hdr_t *hdr)
diff --git a/platform/linux-generic/include/odp_buffer_internal.h b/platform/linux-generic/include/odp_buffer_internal.h
index 076abe96e072..dadf285e796d 100644
--- a/platform/linux-generic/include/odp_buffer_internal.h
+++ b/platform/linux-generic/include/odp_buffer_internal.h
@@ -96,6 +96,9 @@  struct odp_buffer_hdr_t {
 	/* Event type. Maybe different than pool type (crypto compl event) */
 	int8_t    event_type;
 
+	/* Event subtype. Should be ODP_EVENT_NO_SUBTYPE except packets. */
+	int8_t    event_subtype;
+
 	/* Burst table */
 	struct odp_buffer_hdr_t *burst[BUFFER_BURST_SIZE];
 
diff --git a/platform/linux-generic/odp_event.c b/platform/linux-generic/odp_event.c
index d71f4464af48..3cd4a73cbefb 100644
--- a/platform/linux-generic/odp_event.c
+++ b/platform/linux-generic/odp_event.c
@@ -19,6 +19,11 @@  odp_event_type_t odp_event_type(odp_event_t event)
 	return _odp_buffer_event_type(odp_buffer_from_event(event));
 }
 
+odp_event_subtype_t odp_event_subtype(odp_event_t event)
+{
+	return _odp_buffer_event_subtype(odp_buffer_from_event(event));
+}
+
 void odp_event_free(odp_event_t event)
 {
 	switch (odp_event_type(event)) {
diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c
index eb66af2d3b9c..3789feca45f9 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -268,6 +268,7 @@  static inline void packet_init(odp_packet_hdr_t *pkt_hdr, uint32_t len)
 			     CONFIG_PACKET_TAILROOM;
 
 	pkt_hdr->input = ODP_PKTIO_INVALID;
+	pkt_hdr->buf_hdr.event_subtype = ODP_EVENT_PACKET_BASIC;
 }
 
 static inline void init_segments(odp_packet_hdr_t *pkt_hdr[], int num)
diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c
index 9dba734130b4..23b80698d3b0 100644
--- a/platform/linux-generic/odp_pool.c
+++ b/platform/linux-generic/odp_pool.c
@@ -259,6 +259,7 @@  static void init_buffers(pool_t *pool)
 		buf_hdr->size = seg_size;
 		buf_hdr->type = type;
 		buf_hdr->event_type = type;
+		buf_hdr->event_subtype = ODP_EVENT_NO_SUBTYPE;
 		buf_hdr->pool_hdl = pool->pool_hdl;
 		buf_hdr->uarea_addr = uarea;
 		/* Show user requested size through API */
@@ -566,6 +567,16 @@  void _odp_buffer_event_type_set(odp_buffer_t buf, int ev)
 	buf_hdl_to_hdr(buf)->event_type = ev;
 }
 
+odp_event_subtype_t _odp_buffer_event_subtype(odp_buffer_t buf)
+{
+	return buf_hdl_to_hdr(buf)->event_subtype;
+}
+
+void _odp_buffer_event_subtype_set(odp_buffer_t buf, int ev)
+{
+	buf_hdl_to_hdr(buf)->event_subtype = ev;
+}
+
 odp_pool_t odp_pool_lookup(const char *name)
 {
 	uint32_t i;