Re: [lng-odp] [PATCH v2] Misc fast path optimizations
muvarov replied on github web page: platform/linux-generic/include/odp/api/plat/packet_inlines.h line 5 @@ -21,6 +21,9 @@ /** @internal Inline function offsets */ extern const _odp_packet_inline_offset_t _odp_packet_inline; +/** @internal Pool inline function offsets */ +extern const _odp_pool_inline_offset_t _odp_pool_inline; Comment: extern is not needed in .h file, also extern 2 lines above. > Bill Fischofer(Bill-Fischofer-Linaro) wrote: > The API being inlined is `odp_packet_pool()` so this is an artifact of the > implementation of that API. So I'm OK with it being here since this is > supposed to be fastpath code. >> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >> @matiaselo I agree this is correctly placed here. >>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>> 2.0 is based on api-next, not master. This will be one of several conflicts >>> that will need to be resolved at merge time. muvarov wrote @matiaselo yes, I see. While reading patches in that web interface sometime I miss some pieces of code. Just above you changed that. > Matias Elo(matiaselo) wrote: > While packing the pkt_dpdk_t struct I changed pkt_dpdk_t.lockless_rx and > pkt_dpdk_t.lockless_tx to uint8_t. I'm using the same type here. >> muvarov wrote >> why is that needed? >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> This is not planned untill TigerMoth. nagarahalli wrote It is about 2.0 merge to master, it will happen sometime in the future. > Dmitry Eremin-Solenikov(lumag) wrote: > @nagarahalli Any reason why we shouldn't have it in master? >> nagarahalli wrote >> This change is done already in 2.0. >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> @matiaselo ok. Matias Elo(matiaselo) wrote: Since there are currently no functions in sight to put into pool_inlines.h I would suggest not to add a new header file. If/when some pool inline function are added later on the header can be created them. Is this OK for you? > Dmitry Eremin-Solenikov(lumag) wrote: > Just wanted to have pool functions in pool header, etc. >> Matias Elo(matiaselo) wrote: >> I followed the same pattern as the packet inlines. >> _odp_packet_inline_offset_t is defined in packet_types.h. >>> Matias Elo(matiaselo) wrote: >>> pool_inlines.h would only include these two lines and looking >>> at pool API there aren't any additional functions which should >>> be inlined. Do you have a particular use case in mind for this >>> header? Dmitry Eremin-Solenikov(lumag) wrote: And this to pool_inline_types.h. > Dmitry Eremin-Solenikov(lumag) wrote: > This should go to pool_inlines.h https://github.com/Linaro/odp/pull/281#discussion_r150932597 updated_at 2017-11-15 11:47:30
Re: [lng-odp] [PATCH v2] Misc fast path optimizations
muvarov replied on github web page: platform/linux-generic/include/odp/api/plat/packet_inlines.h line 5 @@ -21,6 +21,9 @@ /** @internal Inline function offsets */ extern const _odp_packet_inline_offset_t _odp_packet_inline; +/** @internal Pool inline function offsets */ +extern const _odp_pool_inline_offset_t _odp_pool_inline; Comment: ok, then. > muvarov wrote > ok. I just thought maybe some other implementations already speedup this call > and we can get this benefits for free. But I don't might if it will be simple > memcpy(). >> muvarov wrote >> I think if it will be in this request it will make easy to review all >> changes at once. >>> Matias Elo(matiaselo) wrote: >>> A valid point, at least I don't see any reason why capability struct is >>> stored inside each pktio implementation. MTU is used in the fast path so I >>> wouldn't move that. Should I move the capability in this PR or create a new >>> one? Matias Elo(matiaselo) wrote: The next member in the struct named 'capa' is 40 bytes long, so it will cross a cache line boundary. I was thinking this comment was more useful than an ambiguous cache line comment. There is still space in the first cache line for additional fast path variables, so I wouldn't use a bitmask before it is actually needed. > Matias Elo(matiaselo) wrote: > odp_memcpy() is just a wrapper for memcpy() and I would avoid using api > functions in the implementation unless there is a clear benefit. >> Matias Elo(matiaselo) wrote: >> In this case it is needed. _odp_pool_inline is defined in odp_pool.c and >> _odp_packet_inline in odp_packet.c. ODP build with >> '--disable-abi-compat' will fail without extern declarations. >>> muvarov wrote >>> pktio capability is big struct and it's same for all packets. Is it >>> really needs to be here? as well as uint16_t mtu? why not in >>> pktio_entry_t ? muvarov wrote what is propose of sayng 34 bytes here? Cache line is 64 so maybe comments for cache lines borders are more interesting here? vdev_sysc_promisc, lockless_rx, lockless_tx can be packet to bit field. That will also reduce structure size. Is there any reason to do that? > muvarov wrote > odp_memcpy() might be good fit here. >> muvarov wrote >> extern is not needed in .h file, also extern 2 lines above. >>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>> The API being inlined is `odp_packet_pool()` so this is an artifact >>> of the implementation of that API. So I'm OK with it being here >>> since this is supposed to be fastpath code. Bill Fischofer(Bill-Fischofer-Linaro) wrote: @matiaselo I agree this is correctly placed here. > Bill Fischofer(Bill-Fischofer-Linaro) wrote: > 2.0 is based on api-next, not master. This will be one of several > conflicts that will need to be resolved at merge time. >> muvarov wrote >> @matiaselo yes, I see. While reading patches in that web >> interface sometime I miss some pieces of code. Just above you >> changed that. >>> Matias Elo(matiaselo) wrote: >>> While packing the pkt_dpdk_t struct I changed >>> pkt_dpdk_t.lockless_rx and pkt_dpdk_t.lockless_tx to uint8_t. >>> I'm using the same type here. muvarov wrote why is that needed? > Dmitry Eremin-Solenikov(lumag) wrote: > This is not planned untill TigerMoth. >> nagarahalli wrote >> It is about 2.0 merge to master, it will happen sometime in >> the future. >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> @nagarahalli Any reason why we shouldn't have it in master? nagarahalli wrote This change is done already in 2.0. > Dmitry Eremin-Solenikov(lumag) wrote: > @matiaselo ok. >> Matias Elo(matiaselo) wrote: >> Since there are currently no functions in sight to put >> into pool_inlines.h I would suggest not to add a new >> header file. If/when some pool inline function are added >> later on the header can be created them. Is this OK for >> you? >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> Just wanted to have pool functions in pool header, etc. Matias Elo(matiaselo) wrote: I followed the same pattern as the packet inlines. _odp_packet_inline_offset_t is defined in packet_types.h.
Re: [lng-odp] [PATCH v2] Misc fast path optimizations
muvarov replied on github web page:
platform/linux-generic/include/odp_packet_dpdk.h
@@ -49,16 +49,17 @@ typedef union {
typedef struct {
odp_pool_t pool; /**< pool to alloc packets from */
struct rte_mempool *pkt_pool; /**< DPDK packet pool */
- odp_pktio_capability_t capa; /**< interface capabilities */
uint32_t data_room; /**< maximum packet length */
- uint16_t mtu; /**< maximum transmission unit */
- /** Use system call to get/set vdev promisc mode */
- odp_bool_t vdev_sysc_promisc;
- uint8_t port_id; /**< DPDK port identifier */
unsigned min_rx_burst;/**< minimum RX burst size */
odp_pktin_hash_proto_t hash; /**< Packet input hash protocol */
- odp_bool_t lockless_rx; /**< no locking for rx */
- odp_bool_t lockless_tx; /**< no locking for tx */
+ uint16_t mtu; /**< maximum transmission unit */
+ /** Use system call to get/set vdev promisc mode */
+ uint8_t vdev_sysc_promisc;
+ uint8_t lockless_rx; /**< no locking for rx */
+ uint8_t lockless_tx; /**< no locking for tx */
+ uint8_t port_id; /**< DPDK port identifier */
+ /* --- 34 bytes --- */
+ odp_pktio_capability_t capa;/**< interface capabilities */
Comment:
I think if it will be in this request it will make easy to review all changes
at once.
> Matias Elo(matiaselo) wrote:
> A valid point, at least I don't see any reason why capability struct is
> stored inside each pktio implementation. MTU is used in the fast path so I
> wouldn't move that. Should I move the capability in this PR or create a new
> one?
>> Matias Elo(matiaselo) wrote:
>> The next member in the struct named 'capa' is 40 bytes long, so it will
>> cross a cache line boundary. I was thinking this comment was more useful
>> than an ambiguous cache line comment.
>>
>> There is still space in the first cache line for additional fast path
>> variables, so I wouldn't use a bitmask before it is actually needed.
>>> Matias Elo(matiaselo) wrote:
>>> odp_memcpy() is just a wrapper for memcpy() and I would avoid using api
>>> functions in the implementation unless there is a clear benefit.
Matias Elo(matiaselo) wrote:
In this case it is needed. _odp_pool_inline is defined in odp_pool.c and
_odp_packet_inline in odp_packet.c. ODP build with '--disable-abi-compat'
will fail without extern declarations.
> muvarov wrote
> pktio capability is big struct and it's same for all packets. Is it
> really needs to be here? as well as uint16_t mtu? why not in
> pktio_entry_t ?
>> muvarov wrote
>> what is propose of sayng 34 bytes here? Cache line is 64 so maybe
>> comments for cache lines borders are more interesting here?
>> vdev_sysc_promisc, lockless_rx, lockless_tx can be packet to bit field.
>> That will also reduce structure size. Is there any reason to do that?
>>> muvarov wrote
>>> odp_memcpy() might be good fit here.
muvarov wrote
extern is not needed in .h file, also extern 2 lines above.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The API being inlined is `odp_packet_pool()` so this is an artifact
> of the implementation of that API. So I'm OK with it being here since
> this is supposed to be fastpath code.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> @matiaselo I agree this is correctly placed here.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> 2.0 is based on api-next, not master. This will be one of several
>>> conflicts that will need to be resolved at merge time.
muvarov wrote
@matiaselo yes, I see. While reading patches in that web
interface sometime I miss some pieces of code. Just above you
changed that.
> Matias Elo(matiaselo) wrote:
> While packing the pkt_dpdk_t struct I changed
> pkt_dpdk_t.lockless_rx and pkt_dpdk_t.lockless_tx to uint8_t. I'm
> using the same type here.
>> muvarov wrote
>> why is that needed?
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> This is not planned untill TigerMoth.
nagarahalli wrote
It is about 2.0 merge to master, it will happen sometime in
the future.
> Dmitry Eremin-Solenikov(lumag) wrote:
> @nagarahalli Any reason why we shouldn't have it in master?
>> nagarahalli wrote
>> This change is done already in 2.0.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> @matiaselo ok.
Matias
Re: [lng-odp] [PATCH v2] Misc fast path optimizations
muvarov replied on github web page:
platform/linux-generic/include/odp_packet_dpdk.h
@@ -49,16 +49,17 @@ typedef union {
typedef struct {
odp_pool_t pool; /**< pool to alloc packets from */
struct rte_mempool *pkt_pool; /**< DPDK packet pool */
- odp_pktio_capability_t capa; /**< interface capabilities */
uint32_t data_room; /**< maximum packet length */
- uint16_t mtu; /**< maximum transmission unit */
- /** Use system call to get/set vdev promisc mode */
- odp_bool_t vdev_sysc_promisc;
- uint8_t port_id; /**< DPDK port identifier */
unsigned min_rx_burst;/**< minimum RX burst size */
odp_pktin_hash_proto_t hash; /**< Packet input hash protocol */
- odp_bool_t lockless_rx; /**< no locking for rx */
- odp_bool_t lockless_tx; /**< no locking for tx */
+ uint16_t mtu; /**< maximum transmission unit */
+ /** Use system call to get/set vdev promisc mode */
+ uint8_t vdev_sysc_promisc;
+ uint8_t lockless_rx; /**< no locking for rx */
+ uint8_t lockless_tx; /**< no locking for tx */
+ uint8_t port_id; /**< DPDK port identifier */
+ /* --- 34 bytes --- */
+ odp_pktio_capability_t capa;/**< interface capabilities */
Comment:
pktio capability is big struct and it's same for all packets. Is it really
needs to be here? as well as uint16_t mtu? why not in pktio_entry_t ?
> muvarov wrote
> what is propose of sayng 34 bytes here? Cache line is 64 so maybe comments
> for cache lines borders are more interesting here? vdev_sysc_promisc,
> lockless_rx, lockless_tx can be packet to bit field. That will also reduce
> structure size. Is there any reason to do that?
>> muvarov wrote
>> odp_memcpy() might be good fit here.
>>> muvarov wrote
>>> extern is not needed in .h file, also extern 2 lines above.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
The API being inlined is `odp_packet_pool()` so this is an artifact of the
implementation of that API. So I'm OK with it being here since this is
supposed to be fastpath code.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> @matiaselo I agree this is correctly placed here.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> 2.0 is based on api-next, not master. This will be one of several
>> conflicts that will need to be resolved at merge time.
>>> muvarov wrote
>>> @matiaselo yes, I see. While reading patches in that web interface
>>> sometime I miss some pieces of code. Just above you changed that.
Matias Elo(matiaselo) wrote:
While packing the pkt_dpdk_t struct I changed pkt_dpdk_t.lockless_rx
and pkt_dpdk_t.lockless_tx to uint8_t. I'm using the same type here.
> muvarov wrote
> why is that needed?
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> This is not planned untill TigerMoth.
>>> nagarahalli wrote
>>> It is about 2.0 merge to master, it will happen sometime in the
>>> future.
Dmitry Eremin-Solenikov(lumag) wrote:
@nagarahalli Any reason why we shouldn't have it in master?
> nagarahalli wrote
> This change is done already in 2.0.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> @matiaselo ok.
>>> Matias Elo(matiaselo) wrote:
>>> Since there are currently no functions in sight to put into
>>> pool_inlines.h I would suggest not to add a new header file.
>>> If/when some pool inline function are added later on the header
>>> can be created them. Is this OK for you?
Dmitry Eremin-Solenikov(lumag) wrote:
Just wanted to have pool functions in pool header, etc.
> Matias Elo(matiaselo) wrote:
> I followed the same pattern as the packet inlines.
> _odp_packet_inline_offset_t is defined in packet_types.h.
>> Matias Elo(matiaselo) wrote:
>> pool_inlines.h would only include these two lines and
>> looking at pool API there aren't any additional functions
>> which should be inlined. Do you have a particular use case
>> in mind for this header?
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> And this to pool_inline_types.h.
Dmitry Eremin-Solenikov(lumag) wrote:
This should go to pool_inlines.h
https://github.com/Linaro/odp/pull/281#discussion_r150937775
updated_at 2017-11-15 11:47:30
Re: [lng-odp] [PATCH v2] Misc fast path optimizations
muvarov replied on github web page:
platform/linux-generic/odp_packet.c
line 98
@@ -1780,26 +1778,28 @@ int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt,
odp_packet_t dstpkt)
{
odp_packet_hdr_t *srchdr = packet_hdr(srcpkt);
odp_packet_hdr_t *dsthdr = packet_hdr(dstpkt);
+ pool_t *src_pool = srchdr->buf_hdr.pool_ptr;
+ pool_t *dst_pool = dsthdr->buf_hdr.pool_ptr;
+ uint32_t src_uarea_size = src_pool->params.pkt.uarea_size;
+ uint32_t dst_uarea_size = dst_pool->params.pkt.uarea_size;
dsthdr->input = srchdr->input;
dsthdr->dst_queue = srchdr->dst_queue;
dsthdr->buf_hdr.buf_u64 = srchdr->buf_hdr.buf_u64;
if (dsthdr->buf_hdr.uarea_addr != NULL &&
- srchdr->buf_hdr.uarea_addr != NULL)
- memcpy(dsthdr->buf_hdr.uarea_addr,
- srchdr->buf_hdr.uarea_addr,
- dsthdr->buf_hdr.uarea_size <=
- srchdr->buf_hdr.uarea_size ?
- dsthdr->buf_hdr.uarea_size :
- srchdr->buf_hdr.uarea_size);
+ srchdr->buf_hdr.uarea_addr != NULL) {
+ memcpy(dsthdr->buf_hdr.uarea_addr, srchdr->buf_hdr.uarea_addr,
Comment:
odp_memcpy() might be good fit here.
> muvarov wrote
> extern is not needed in .h file, also extern 2 lines above.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> The API being inlined is `odp_packet_pool()` so this is an artifact of the
>> implementation of that API. So I'm OK with it being here since this is
>> supposed to be fastpath code.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> @matiaselo I agree this is correctly placed here.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
2.0 is based on api-next, not master. This will be one of several
conflicts that will need to be resolved at merge time.
> muvarov wrote
> @matiaselo yes, I see. While reading patches in that web interface
> sometime I miss some pieces of code. Just above you changed that.
>> Matias Elo(matiaselo) wrote:
>> While packing the pkt_dpdk_t struct I changed pkt_dpdk_t.lockless_rx and
>> pkt_dpdk_t.lockless_tx to uint8_t. I'm using the same type here.
>>> muvarov wrote
>>> why is that needed?
Dmitry Eremin-Solenikov(lumag) wrote:
This is not planned untill TigerMoth.
> nagarahalli wrote
> It is about 2.0 merge to master, it will happen sometime in the
> future.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> @nagarahalli Any reason why we shouldn't have it in master?
>>> nagarahalli wrote
>>> This change is done already in 2.0.
Dmitry Eremin-Solenikov(lumag) wrote:
@matiaselo ok.
> Matias Elo(matiaselo) wrote:
> Since there are currently no functions in sight to put into
> pool_inlines.h I would suggest not to add a new header file.
> If/when some pool inline function are added later on the header
> can be created them. Is this OK for you?
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Just wanted to have pool functions in pool header, etc.
>>> Matias Elo(matiaselo) wrote:
>>> I followed the same pattern as the packet inlines.
>>> _odp_packet_inline_offset_t is defined in packet_types.h.
Matias Elo(matiaselo) wrote:
pool_inlines.h would only include these two lines and looking
at pool API there aren't any additional functions which should
be inlined. Do you have a particular use case in mind for this
header?
> Dmitry Eremin-Solenikov(lumag) wrote:
> And this to pool_inline_types.h.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> This should go to pool_inlines.h
https://github.com/Linaro/odp/pull/281#discussion_r150933232
updated_at 2017-11-15 11:47:30
Re: [lng-odp] [PATCH v2] Misc fast path optimizations
Dmitry Eremin-Solenikov(lumag) replied on github web page: platform/linux-generic/include/odp/api/plat/packet_inlines.h line 5 @@ -21,6 +21,9 @@ /** @internal Inline function offsets */ extern const _odp_packet_inline_offset_t _odp_packet_inline; +/** @internal Pool inline function offsets */ +extern const _odp_pool_inline_offset_t _odp_pool_inline; Comment: @muvarov extern is not needed for function prototypes. > muvarov wrote > ok, then. >> muvarov wrote >> ok. I just thought maybe some other implementations already speedup this >> call and we can get this benefits for free. But I don't might if it will be >> simple memcpy(). >>> muvarov wrote >>> I think if it will be in this request it will make easy to review all >>> changes at once. Matias Elo(matiaselo) wrote: A valid point, at least I don't see any reason why capability struct is stored inside each pktio implementation. MTU is used in the fast path so I wouldn't move that. Should I move the capability in this PR or create a new one? > Matias Elo(matiaselo) wrote: > The next member in the struct named 'capa' is 40 bytes long, so it will > cross a cache line boundary. I was thinking this comment was more useful > than an ambiguous cache line comment. > > There is still space in the first cache line for additional fast path > variables, so I wouldn't use a bitmask before it is actually needed. >> Matias Elo(matiaselo) wrote: >> odp_memcpy() is just a wrapper for memcpy() and I would avoid using api >> functions in the implementation unless there is a clear benefit. >>> Matias Elo(matiaselo) wrote: >>> In this case it is needed. _odp_pool_inline is defined in odp_pool.c >>> and _odp_packet_inline in odp_packet.c. ODP build with >>> '--disable-abi-compat' will fail without extern declarations. muvarov wrote pktio capability is big struct and it's same for all packets. Is it really needs to be here? as well as uint16_t mtu? why not in pktio_entry_t ? > muvarov wrote > what is propose of sayng 34 bytes here? Cache line is 64 so maybe > comments for cache lines borders are more interesting here? > vdev_sysc_promisc, lockless_rx, lockless_tx can be packet to bit > field. That will also reduce structure size. Is there any reason to > do that? >> muvarov wrote >> odp_memcpy() might be good fit here. >>> muvarov wrote >>> extern is not needed in .h file, also extern 2 lines above. Bill Fischofer(Bill-Fischofer-Linaro) wrote: The API being inlined is `odp_packet_pool()` so this is an artifact of the implementation of that API. So I'm OK with it being here since this is supposed to be fastpath code. > Bill Fischofer(Bill-Fischofer-Linaro) wrote: > @matiaselo I agree this is correctly placed here. >> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >> 2.0 is based on api-next, not master. This will be one of >> several conflicts that will need to be resolved at merge time. >>> muvarov wrote >>> @matiaselo yes, I see. While reading patches in that web >>> interface sometime I miss some pieces of code. Just above you >>> changed that. Matias Elo(matiaselo) wrote: While packing the pkt_dpdk_t struct I changed pkt_dpdk_t.lockless_rx and pkt_dpdk_t.lockless_tx to uint8_t. I'm using the same type here. > muvarov wrote > why is that needed? >> Dmitry Eremin-Solenikov(lumag) wrote: >> This is not planned untill TigerMoth. >>> nagarahalli wrote >>> It is about 2.0 merge to master, it will happen sometime in >>> the future. Dmitry Eremin-Solenikov(lumag) wrote: @nagarahalli Any reason why we shouldn't have it in master? > nagarahalli wrote > This change is done already in 2.0. >> Dmitry Eremin-Solenikov(lumag) wrote: >> @matiaselo ok. >>> Matias Elo(matiaselo) wrote: >>> Since there are currently no functions in sight to put >>> into pool_inlines.h I would suggest not to add a new >>> header file. If/when some pool inline function are >>> added later on the header can be created them. Is this >>> OK for you? Dmitry Eremin-Solenikov(lumag) wrote: Just wanted to have pool functions in pool header, etc. > Matias Elo(matiaselo) wrote:
Re: [lng-odp] [PATCH v2] Misc fast path optimizations
muvarov replied on github web page:
platform/linux-generic/include/odp_packet_dpdk.h
line 20
@@ -49,16 +49,17 @@ typedef union {
typedef struct {
odp_pool_t pool; /**< pool to alloc packets from */
struct rte_mempool *pkt_pool; /**< DPDK packet pool */
- odp_pktio_capability_t capa; /**< interface capabilities */
uint32_t data_room; /**< maximum packet length */
- uint16_t mtu; /**< maximum transmission unit */
- /** Use system call to get/set vdev promisc mode */
- odp_bool_t vdev_sysc_promisc;
- uint8_t port_id; /**< DPDK port identifier */
unsigned min_rx_burst;/**< minimum RX burst size */
odp_pktin_hash_proto_t hash; /**< Packet input hash protocol */
- odp_bool_t lockless_rx; /**< no locking for rx */
- odp_bool_t lockless_tx; /**< no locking for tx */
+ uint16_t mtu; /**< maximum transmission unit */
+ /** Use system call to get/set vdev promisc mode */
+ uint8_t vdev_sysc_promisc;
+ uint8_t lockless_rx; /**< no locking for rx */
+ uint8_t lockless_tx; /**< no locking for tx */
+ uint8_t port_id; /**< DPDK port identifier */
+ /* --- 34 bytes --- */
Comment:
what is propose of sayng 34 bytes here? Cache line is 64 so maybe comments for
cache lines borders are more interesting here? vdev_sysc_promisc, lockless_rx,
lockless_tx can be packet to bit field. That will also reduce structure size.
Is there any reason to do that?
> muvarov wrote
> odp_memcpy() might be good fit here.
>> muvarov wrote
>> extern is not needed in .h file, also extern 2 lines above.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> The API being inlined is `odp_packet_pool()` so this is an artifact of the
>>> implementation of that API. So I'm OK with it being here since this is
>>> supposed to be fastpath code.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
@matiaselo I agree this is correctly placed here.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> 2.0 is based on api-next, not master. This will be one of several
> conflicts that will need to be resolved at merge time.
>> muvarov wrote
>> @matiaselo yes, I see. While reading patches in that web interface
>> sometime I miss some pieces of code. Just above you changed that.
>>> Matias Elo(matiaselo) wrote:
>>> While packing the pkt_dpdk_t struct I changed pkt_dpdk_t.lockless_rx
>>> and pkt_dpdk_t.lockless_tx to uint8_t. I'm using the same type here.
muvarov wrote
why is that needed?
> Dmitry Eremin-Solenikov(lumag) wrote:
> This is not planned untill TigerMoth.
>> nagarahalli wrote
>> It is about 2.0 merge to master, it will happen sometime in the
>> future.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> @nagarahalli Any reason why we shouldn't have it in master?
nagarahalli wrote
This change is done already in 2.0.
> Dmitry Eremin-Solenikov(lumag) wrote:
> @matiaselo ok.
>> Matias Elo(matiaselo) wrote:
>> Since there are currently no functions in sight to put into
>> pool_inlines.h I would suggest not to add a new header file.
>> If/when some pool inline function are added later on the header
>> can be created them. Is this OK for you?
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Just wanted to have pool functions in pool header, etc.
Matias Elo(matiaselo) wrote:
I followed the same pattern as the packet inlines.
_odp_packet_inline_offset_t is defined in packet_types.h.
> Matias Elo(matiaselo) wrote:
> pool_inlines.h would only include these two lines and looking
> at pool API there aren't any additional functions which
> should be inlined. Do you have a particular use case in mind
> for this header?
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> And this to pool_inline_types.h.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> This should go to pool_inlines.h
https://github.com/Linaro/odp/pull/281#discussion_r150935160
updated_at 2017-11-15 11:47:30
Re: [lng-odp] [PATCH v2] Misc fast path optimizations
Matias Elo(matiaselo) replied on github web page: platform/linux-generic/include/odp/api/plat/packet_inlines.h line 5 @@ -21,6 +21,9 @@ /** @internal Inline function offsets */ extern const _odp_packet_inline_offset_t _odp_packet_inline; +/** @internal Pool inline function offsets */ +extern const _odp_pool_inline_offset_t _odp_pool_inline; Comment: In this case it is needed. _odp_pool_inline is defined in odp_pool.c and _odp_packet_inline in odp_packet.c. ODP build with '--disable-abi-compat' will fail without extern declarations. > muvarov wrote > pktio capability is big struct and it's same for all packets. Is it really > needs to be here? as well as uint16_t mtu? why not in pktio_entry_t ? >> muvarov wrote >> what is propose of sayng 34 bytes here? Cache line is 64 so maybe comments >> for cache lines borders are more interesting here? vdev_sysc_promisc, >> lockless_rx, lockless_tx can be packet to bit field. That will also reduce >> structure size. Is there any reason to do that? >>> muvarov wrote >>> odp_memcpy() might be good fit here. muvarov wrote extern is not needed in .h file, also extern 2 lines above. > Bill Fischofer(Bill-Fischofer-Linaro) wrote: > The API being inlined is `odp_packet_pool()` so this is an artifact of > the implementation of that API. So I'm OK with it being here since this > is supposed to be fastpath code. >> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >> @matiaselo I agree this is correctly placed here. >>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>> 2.0 is based on api-next, not master. This will be one of several >>> conflicts that will need to be resolved at merge time. muvarov wrote @matiaselo yes, I see. While reading patches in that web interface sometime I miss some pieces of code. Just above you changed that. > Matias Elo(matiaselo) wrote: > While packing the pkt_dpdk_t struct I changed pkt_dpdk_t.lockless_rx > and pkt_dpdk_t.lockless_tx to uint8_t. I'm using the same type here. >> muvarov wrote >> why is that needed? >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> This is not planned untill TigerMoth. nagarahalli wrote It is about 2.0 merge to master, it will happen sometime in the future. > Dmitry Eremin-Solenikov(lumag) wrote: > @nagarahalli Any reason why we shouldn't have it in master? >> nagarahalli wrote >> This change is done already in 2.0. >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> @matiaselo ok. Matias Elo(matiaselo) wrote: Since there are currently no functions in sight to put into pool_inlines.h I would suggest not to add a new header file. If/when some pool inline function are added later on the header can be created them. Is this OK for you? > Dmitry Eremin-Solenikov(lumag) wrote: > Just wanted to have pool functions in pool header, etc. >> Matias Elo(matiaselo) wrote: >> I followed the same pattern as the packet inlines. >> _odp_packet_inline_offset_t is defined in packet_types.h. >>> Matias Elo(matiaselo) wrote: >>> pool_inlines.h would only include these two lines and >>> looking at pool API there aren't any additional functions >>> which should be inlined. Do you have a particular use case >>> in mind for this header? Dmitry Eremin-Solenikov(lumag) wrote: And this to pool_inline_types.h. > Dmitry Eremin-Solenikov(lumag) wrote: > This should go to pool_inlines.h https://github.com/Linaro/odp/pull/281#discussion_r151049891 updated_at 2017-11-15 11:47:30
Re: [lng-odp] [PATCH v2] Misc fast path optimizations
muvarov replied on github web page: platform/linux-generic/include/odp/api/plat/packet_inlines.h line 5 @@ -21,6 +21,9 @@ /** @internal Inline function offsets */ extern const _odp_packet_inline_offset_t _odp_packet_inline; +/** @internal Pool inline function offsets */ +extern const _odp_pool_inline_offset_t _odp_pool_inline; Comment: extern is not needed in .h file, also extern 2 lines above. > Bill Fischofer(Bill-Fischofer-Linaro) wrote: > The API being inlined is `odp_packet_pool()` so this is an artifact of the > implementation of that API. So I'm OK with it being here since this is > supposed to be fastpath code. >> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >> @matiaselo I agree this is correctly placed here. >>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>> 2.0 is based on api-next, not master. This will be one of several conflicts >>> that will need to be resolved at merge time. muvarov wrote @matiaselo yes, I see. While reading patches in that web interface sometime I miss some pieces of code. Just above you changed that. > Matias Elo(matiaselo) wrote: > While packing the pkt_dpdk_t struct I changed pkt_dpdk_t.lockless_rx and > pkt_dpdk_t.lockless_tx to uint8_t. I'm using the same type here. >> muvarov wrote >> why is that needed? >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> This is not planned untill TigerMoth. nagarahalli wrote It is about 2.0 merge to master, it will happen sometime in the future. > Dmitry Eremin-Solenikov(lumag) wrote: > @nagarahalli Any reason why we shouldn't have it in master? >> nagarahalli wrote >> This change is done already in 2.0. >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> @matiaselo ok. Matias Elo(matiaselo) wrote: Since there are currently no functions in sight to put into pool_inlines.h I would suggest not to add a new header file. If/when some pool inline function are added later on the header can be created them. Is this OK for you? > Dmitry Eremin-Solenikov(lumag) wrote: > Just wanted to have pool functions in pool header, etc. >> Matias Elo(matiaselo) wrote: >> I followed the same pattern as the packet inlines. >> _odp_packet_inline_offset_t is defined in packet_types.h. >>> Matias Elo(matiaselo) wrote: >>> pool_inlines.h would only include these two lines and looking >>> at pool API there aren't any additional functions which should >>> be inlined. Do you have a particular use case in mind for this >>> header? Dmitry Eremin-Solenikov(lumag) wrote: And this to pool_inline_types.h. > Dmitry Eremin-Solenikov(lumag) wrote: > This should go to pool_inlines.h https://github.com/Linaro/odp/pull/281#discussion_r150932597 updated_at 2017-11-15 11:47:30
