On 15.02.2019 21:48, Flavio Leitner wrote:
> On Fri, Feb 15, 2019 at 04:07:02PM +0300, 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_offload_invalidate' 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__()'.
>>
>> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
>> ---
>> lib/dp-packet.c | 14 ++++-------
>> lib/dp-packet.h | 62 ++++++++++++++---------------------------------
>> lib/netdev-dpdk.c | 6 ++---
>> lib/netdev.c | 4 +--
>> 4 files changed, 28 insertions(+), 58 deletions(-)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 93b0e9c84..b5942f815 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -30,8 +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);
>> +#ifdef DPDK_NETDEV
>> dp_packet_mbuf_init(b);
>> +#endif
>
> Can we call it something generic and have it defined for both dpdk
> and non-dpdk like other functions?
>
> E.g.: dp_packet_buffer_init()?
This is a bit tricky. I don't think that "buffer" is a right word here
because we're initializing some specific mbuf fields that are more like
fields of dp_packet structure and not the part of the data buffer.
IMHO, 'dp_packet_buffer_init' is confusing.
Maybe something like:
/* Extra initialization for implementation-specific fields of dp-packet. */
static inline void dp_packet_init_specific(struct dp_packet *p);
What do you think ?
>
>
>> + dp_packet_offload_invalidate(b);
>
> Like that though I think the name could be dp_packet_offload_reset()
> but no strong opinion here.
That's a good suggestion. However, I'd like it to be 'dp_packet_reset_offload()'
to follow common naming pattern of the dp_packet functions. Ex.:
dp_packet_reset_offsets()
dp_packet_reset_cutlen()
dp_packet_reset_packet()
>
>> dp_packet_reset_cutlen(b);
>> /* By default assume the packet type to be Ethernet. */
>> b->packet_type = htonl(PT_ETH);
>> @@ -173,16 +175,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));
>> }
>
> nice
>
>
>>
>> return new_buffer;
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index c6672f6be..685edc90c 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -58,8 +58,9 @@ struct dp_packet {
>> uint16_t allocated_; /* Number of bytes allocated. */
>> uint16_t data_ofs; /* First byte actually in use. */
>> uint32_t size_; /* Number of bytes in use. */
>> + uint32_t ol_flags; /* Offloading flags. */
>> +#define DP_PACKET_OL_RSS_HASH_MASK 0x1 /* Is the 'rss_hash' valid? */
>> uint32_t rss_hash; /* Packet hash. */
>> - bool rss_hash_valid; /* Is the 'rss_hash' valid? */
>> #endif
>> enum dp_packet_source source; /* Source of memory allocated as 'base'.
>> */
>>
>> @@ -421,6 +422,16 @@ dp_packet_get_nd_payload(const struct dp_packet *b)
>> #ifdef DPDK_NETDEV
>> BUILD_ASSERT_DECL(offsetof(struct dp_packet, mbuf) == 0);
>>
>> +/* This initialization is needed for packets that do not come from DPDK
>> + * interfaces, when vswitchd is built with --with-dpdk. */
>> +static inline void
>> +dp_packet_mbuf_init(struct dp_packet *p)
>> +{
>> + p->mbuf.tx_offload = p->mbuf.packet_type = 0;
>> + p->mbuf.nb_segs = 1;
>> + p->mbuf.next = NULL;
>> +}
>> +
>> static inline void *
>> dp_packet_base(const struct dp_packet *b)
>> {
>> @@ -501,24 +512,9 @@ dp_packet_rss_valid(const struct dp_packet *p)
>> }
>>
>> static inline void
>> -dp_packet_rss_invalidate(struct dp_packet *p OVS_UNUSED)
>> -{
>> -}
>> -
>> -static inline void
>> -dp_packet_mbuf_rss_flag_reset(struct dp_packet *p)
>> -{
>> - p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
>> -}
>> -
>> -/* This initialization is needed for packets that do not come from DPDK
>> - * interfaces, when vswitchd is built with --with-dpdk. */
>> -static inline void
>> -dp_packet_mbuf_init(struct dp_packet *p)
>> +dp_packet_offload_invalidate(struct dp_packet *p OVS_UNUSED)
>> {
> ^^^^^^^^^^^
> Looks like p is used below to set ol_flags.
Thanks. I'll fix that.
>
> Other parts look ok, though it seems like this could be split in two
> patches. Just a suggestion.
I'd like to keep it as a single change to not modify same code in a two
subsequent patches or having duplicated functionality at some point in time.
> Thanks,
> fbl
>
>
>> - p->mbuf.ol_flags = p->mbuf.tx_offload = p->mbuf.packet_type = 0;
>> - p->mbuf.nb_segs = 1;
>> - p->mbuf.next = NULL;
>> + p->mbuf.ol_flags = 0;
>> }
>>
>> static inline bool
>> @@ -549,13 +545,6 @@ dp_packet_l4_checksum_bad(const struct dp_packet *p)
>> PKT_RX_L4_CKSUM_BAD;
>> }
>>
>> -static inline void
>> -reset_dp_packet_checksum_ol_flags(struct dp_packet *p)
>> -{
>> - p->mbuf.ol_flags &= ~(PKT_RX_L4_CKSUM_GOOD | PKT_RX_L4_CKSUM_BAD |
>> - PKT_RX_IP_CKSUM_GOOD | PKT_RX_IP_CKSUM_BAD);
>> -}
>> -
>> static inline bool
>> dp_packet_has_flow_mark(const struct dp_packet *p, uint32_t *mark)
>> {
>> @@ -628,29 +617,19 @@ static inline void
>> dp_packet_set_rss_hash(struct dp_packet *p, uint32_t hash)
>> {
>> p->rss_hash = hash;
>> - p->rss_hash_valid = true;
>> + p->ol_flags |= DP_PACKET_OL_RSS_HASH_MASK;
>> }
>>
>> static inline bool
>> dp_packet_rss_valid(const struct dp_packet *p)
>> {
>> - return p->rss_hash_valid;
>> + return p->ol_flags & DP_PACKET_OL_RSS_HASH_MASK;
>> }
>>
>> static inline void
>> -dp_packet_rss_invalidate(struct dp_packet *p)
>> -{
>> - p->rss_hash_valid = false;
>> -}
>> -
>> -static inline void
>> -dp_packet_mbuf_rss_flag_reset(struct dp_packet *p OVS_UNUSED)
>> -{
>> -}
>> -
>> -static inline void
>> -dp_packet_mbuf_init(struct dp_packet *p OVS_UNUSED)
>> +dp_packet_offload_invalidate(struct dp_packet *p)
>> {
>> + p->ol_flags = 0;
>> }
>>
>> static inline bool
>> @@ -677,11 +656,6 @@ dp_packet_l4_checksum_bad(const struct dp_packet *p
>> OVS_UNUSED)
>> return false;
>> }
>>
>> -static inline void
>> -reset_dp_packet_checksum_ol_flags(struct dp_packet *p OVS_UNUSED)
>> -{
>> -}
>> -
>> static inline bool
>> dp_packet_has_flow_mark(const struct dp_packet *p OVS_UNUSED,
>> uint32_t *mark OVS_UNUSED)
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index f07b10c88..26022a59c 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -3774,11 +3774,11 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,
>> struct dp_packet *packet;
>>
>> /* When using 'dpdkr' and sending to a DPDK ring, we want to ensure that
>> - * the rss hash field is clear. This is because the same mbuf may be
>> + * the offload fields are clear. This is because the same mbuf may be
>> * modified by the consumer of the ring and return into the datapath
>> - * without recalculating the RSS hash. */
>> + * without recalculating the RSS hash or revalidating the checksums. */
>> DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
>> - dp_packet_mbuf_rss_flag_reset(packet);
>> + dp_packet_offload_invalidate(packet);
>> }
>>
>> netdev_dpdk_send__(dev, qid, batch, concurrent_txq);
>> diff --git a/lib/netdev.c b/lib/netdev.c
>> index 45b50f26c..4f26515dc 100644
>> --- a/lib/netdev.c
>> +++ b/lib/netdev.c
>> @@ -814,10 +814,10 @@ netdev_pop_header(struct netdev *netdev, struct
>> dp_packet_batch *batch)
>> DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
>> packet = netdev->netdev_class->pop_header(packet);
>> if (packet) {
>> - /* Reset the checksum offload flags if present, to avoid wrong
>> + /* Reset the offload flags if present, to avoid wrong
>> * interpretation in the further packet processing when
>> * recirculated.*/
>> - reset_dp_packet_checksum_ol_flags(packet);
>> + dp_packet_offload_invalidate(packet);
>> dp_packet_batch_refill(batch, packet, i);
>> }
>> }
>> --
>> 2.17.1
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
