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. >>>>>>>>>>>>>>>>>>>>>>>>> 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_r151062646 updated_at 2017-11-15 11:47:30
