Matias Elo(matiaselo) 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:
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_r151059106
updated_at 2017-11-15 11:47:30
