On 13.03.2019 13:09, Ian Stokes wrote:
> On 2/26/2019 10:38 AM, Ilya Maximets wrote:
>> 1. No reason to have mbuf related APIs in a generic code.
>> 2. Not only RSS/checksums should be invalidated in case of tunnel
>> decapsulation or sending to 'ring' ports.
>>
>> In order to fix two above issues, new function
>> 'dp_packet_reset_offload' introduced. In order to clean up/unify
>> the code and simplify addition of new offloading features to non-DPDK
>> version of dp_packet, introduced 'ol_flags' bitmask. Additionally
>> reduced code complexity in 'dp_packet_clone_with_headroom' by using
>> already existent generic APIs.
>> > Unfortunately, we still need to have a special case for mbuf
>> initialization inside 'dp_packet_init__()'.
>> 'dp_packet_init_specific()' introduced for this purpose as a generic
>> API for initialization of the implementation-specific fields.
>>
>
> Thanks for this Ilya, apologies for the delay with the series, should have
> more time to look at it this week.
>
> One minor comment below, but other than that seems ok.
>
>> Acked-by: Flavio Leitner <f...@sysclose.org>
>> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
>> ---
>> lib/dp-packet.c | 15 ++++------
>> lib/dp-packet.h | 75 ++++++++++++++++++++---------------------------
>> lib/netdev-dpdk.c | 6 ++--
>> lib/netdev.c | 4 +--
>> 4 files changed, 41 insertions(+), 59 deletions(-)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 93b0e9c84..f8207ffc2 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -30,9 +30,10 @@ dp_packet_init__(struct dp_packet *b, size_t allocated,
>> enum dp_packet_source so
>> b->source = source;
>> dp_packet_reset_offsets(b);
>> pkt_metadata_init(&b->md, 0);
>> - dp_packet_rss_invalidate(b);
>> - dp_packet_mbuf_init(b);
>> dp_packet_reset_cutlen(b);
>> + dp_packet_reset_offload(b);
>> + /* Initialize implementation-specific fields of dp_packet. */
>> + dp_packet_init_specific(b);
>> /* By default assume the packet type to be Ethernet. */
>> b->packet_type = htonl(PT_ETH);
>> }
>> @@ -173,16 +174,10 @@ dp_packet_clone_with_headroom(const struct dp_packet
>> *buffer, size_t headroom)
>> #ifdef DPDK_NETDEV
>> new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
>> -#else
>> - new_buffer->rss_hash_valid = buffer->rss_hash_valid;
>> #endif
>> - if (dp_packet_rss_valid(new_buffer)) {
>> -#ifdef DPDK_NETDEV
>> - new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss;
>> -#else
>> - new_buffer->rss_hash = buffer->rss_hash;
>> -#endif
>> + if (dp_packet_rss_valid(buffer)) {
>> + dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer));
>> }
>> return new_buffer;
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index c6672f6be..b34dada78 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -46,6 +46,13 @@ enum OVS_PACKED_ENUM dp_packet_source {
>> #define DP_PACKET_CONTEXT_SIZE 64
>> +#ifndef DPDK_NETDEV
>> +/* Bit masks for the 'ol_flags' member of the 'dp_packet' structure. */
>> +enum dp_packet_offload_mask {
>> + DP_PACKET_OL_RSS_HASH_MASK = 0x1, /* Is the 'rss_hash' valid? */
>> +};
>
> Is there a specific reason you've used an enum instead of a define here as
> its just a single value.
>
> Is the expectation that the entries for the offload mask will be expanded in
> the future to be more than just RSS_HASH_MASK?
Sure. Next patch adds the DP_PACKET_OL_FLOW_MARK_MASK.
It's used by netdev-dummy partial offloading support.
>
> Ian
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
