| Message ID | 20190215130708.8608-4-i.maximets@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dpif-netdev: Partial HWOL fixes/refactoring/unit-tests. | expand |
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.maximets@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()? > + dp_packet_offload_invalidate(b); Like that though I think the name could be dp_packet_offload_reset() but no strong opinion here. > 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. Other parts look ok, though it seems like this could be split in two patches. Just a suggestion. 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Regards, Asaf Penso > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org <ovs-dev- > bounces@openvswitch.org> On Behalf Of Ilya Maximets > Sent: Friday, February 15, 2019 3:07 PM > To: ovs-dev@openvswitch.org; Ian Stokes <ian.stokes@intel.com> > Cc: Roni Bar Yanai <roniba@mellanox.com>; Flavio Leitner > <fbl@sysclose.org>; Ilya Maximets <i.maximets@samsung.com> > Subject: [ovs-dev] [PATCH v4 3/9] dp-packet: Refactor offloading API. > > 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.maximets@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 > + dp_packet_offload_invalidate(b); > 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)); > } > > 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. */ I think we can consider even now to use 64bit of 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) > { > - 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 > dev@openvswitch.org > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma > il.openvswitch.org%2Fmailman%2Flistinfo%2Fovs- > dev&data=02%7C01%7Casafp%40mellanox.com%7Ce1f7df77aa754e663 > c4708d69346d135%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63 > 6858329718367951&sdata=LimdEf4hzLJxXAe%2BacTi9PFD5Udu1og5IuA > DJVoAxkk%3D&reserved=0
> -----Original Message----- > From: Ilya Maximets <i.maximets@samsung.com> > Sent: Friday, February 15, 2019 3:07 PM > To: ovs-dev@openvswitch.org; Ian Stokes <ian.stokes@intel.com> > Cc: Flavio Leitner <fbl@sysclose.org>; Ophir Munk > <ophirmu@mellanox.com>; Kevin Traynor <ktraynor@redhat.com>; Roni Bar > Yanai <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ilya > Maximets <i.maximets@samsung.com> > Subject: [PATCH v4 3/9] dp-packet: Refactor offloading API. > > 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__()'. Do you mean by 'special case' that calling dp_packet_mbuf_init was under #ifdef DPDK_NETDEV? Please note that prior to this commit: 1. 'dp_packet_mbuf_init' was defined twice in dp-packet.h. Once under #ifdef DPDK_NETDEV and a second time under the #else with an empty body. 2. There was no #ifdef in dp_packet_init__(). This commit removes the second implementation of 'dp_packet_mbuf_init' (with the empty body) and adds #ifdef in dp_packet_init__(). +#ifdef DPDK_NETDEV dp_packet_mbuf_init(b); +#endif Can you please explain? What happens if you restore 'dp_packet_mbuf_init' with empty body and remove the #ifdef in dp_packet_init__()?
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.maximets@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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On 17.02.2019 16:37, Asaf Penso wrote: > > > Regards, > Asaf Penso > >> -----Original Message----- >> From: ovs-dev-bounces@openvswitch.org <ovs-dev- >> bounces@openvswitch.org> On Behalf Of Ilya Maximets >> Sent: Friday, February 15, 2019 3:07 PM >> To: ovs-dev@openvswitch.org; Ian Stokes <ian.stokes@intel.com> >> Cc: Roni Bar Yanai <roniba@mellanox.com>; Flavio Leitner >> <fbl@sysclose.org>; Ilya Maximets <i.maximets@samsung.com> >> Subject: [ovs-dev] [PATCH v4 3/9] dp-packet: Refactor offloading API. >> >> 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.maximets@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 >> + dp_packet_offload_invalidate(b); >> 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)); >> } >> >> 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. */ > > I think we can consider even now to use 64bit of flags. I don't think so because we have only 2 bits used. If we'll need more than 32 offloading features implemented in OVS we'll likely need different infrastructure for handling them. Increasing the size would be much easier than implementing new handlers. Also, we have no any ABI for dp_packet, it's internal structure and could be changed anytime without issues. No need to eat more memory. > >> +#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) >> { >> - 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 >> dev@openvswitch.org >> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma >> il.openvswitch.org%2Fmailman%2Flistinfo%2Fovs- >> dev&data=02%7C01%7Casafp%40mellanox.com%7Ce1f7df77aa754e663 >> c4708d69346d135%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63 >> 6858329718367951&sdata=LimdEf4hzLJxXAe%2BacTi9PFD5Udu1og5IuA >> DJVoAxkk%3D&reserved=0 > >
On 17.02.2019 20:00, Ophir Munk wrote: > > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: Friday, February 15, 2019 3:07 PM >> To: ovs-dev@openvswitch.org; Ian Stokes <ian.stokes@intel.com> >> Cc: Flavio Leitner <fbl@sysclose.org>; Ophir Munk >> <ophirmu@mellanox.com>; Kevin Traynor <ktraynor@redhat.com>; Roni Bar >> Yanai <roniba@mellanox.com>; Finn Christensen <fc@napatech.com>; Ilya >> Maximets <i.maximets@samsung.com> >> Subject: [PATCH v4 3/9] dp-packet: Refactor offloading API. >> >> 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__()'. > > Do you mean by 'special case' that calling dp_packet_mbuf_init was under #ifdef DPDK_NETDEV? > Please note that prior to this commit: > 1. 'dp_packet_mbuf_init' was defined twice in dp-packet.h. > Once under #ifdef DPDK_NETDEV and a second time under the #else with an empty body. > 2. There was no #ifdef in dp_packet_init__(). > > This commit removes the second implementation of 'dp_packet_mbuf_init' (with the empty body) and adds #ifdef in dp_packet_init__(). > +#ifdef DPDK_NETDEV > dp_packet_mbuf_init(b); > +#endif > > Can you please explain? > What happens if you restore 'dp_packet_mbuf_init' with empty body and remove the #ifdef in dp_packet_init__()? By the 'special case' I meant calling something mbuf specific, i.e. something that contains 'mbuf' in it's name, from the generic code. As I wrote in reply to Flavio, we could rename this to something like 'dp_packet_init_specific()' and define in both DPDK and non-DPDK cases to make it more generic. Best regards, Ilya Maximets.
Regards, Asaf Penso > -----Original Message----- > From: Ilya Maximets <i.maximets@samsung.com> > Sent: Monday, February 18, 2019 2:10 PM > To: Asaf Penso <asafp@mellanox.com>; ovs-dev@openvswitch.org; Ian > Stokes <ian.stokes@intel.com> > Cc: Roni Bar Yanai <roniba@mellanox.com>; Flavio Leitner > <fbl@sysclose.org> > Subject: Re: [ovs-dev] [PATCH v4 3/9] dp-packet: Refactor offloading API. > > On 17.02.2019 16:37, Asaf Penso wrote: > > > > > > Regards, > > Asaf Penso > > > >> -----Original Message----- > >> From: ovs-dev-bounces@openvswitch.org <ovs-dev- > >> bounces@openvswitch.org> On Behalf Of Ilya Maximets > >> Sent: Friday, February 15, 2019 3:07 PM > >> To: ovs-dev@openvswitch.org; Ian Stokes <ian.stokes@intel.com> > >> Cc: Roni Bar Yanai <roniba@mellanox.com>; Flavio Leitner > >> <fbl@sysclose.org>; Ilya Maximets <i.maximets@samsung.com> > >> Subject: [ovs-dev] [PATCH v4 3/9] dp-packet: Refactor offloading API. > >> > >> 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.maximets@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 > >> + dp_packet_offload_invalidate(b); > >> 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)); > >> } > >> > >> 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. */ > > > > I think we can consider even now to use 64bit of flags. > > I don't think so because we have only 2 bits used. If we'll need more than 32 > offloading features implemented in OVS we'll likely need different > infrastructure for handling them. Increasing the size would be much easier > than implementing new handlers. Also, we have no any ABI for dp_packet, > it's internal structure and could be changed anytime without issues. > No need to eat more memory. > OK, I agree, thanks. > > > >> +#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) > >> { > >> - 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 > >> dev@openvswitch.org > >> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma > >> il.openvswitch.org%2Fmailman%2Flistinfo%2Fovs- > >> > dev&data=02%7C01%7Casafp%40mellanox.com%7Ce1f7df77aa754e663 > >> > c4708d69346d135%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63 > >> > 6858329718367951&sdata=LimdEf4hzLJxXAe%2BacTi9PFD5Udu1og5IuA > >> DJVoAxkk%3D&reserved=0 > > > >
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 + dp_packet_offload_invalidate(b); 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)); } 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) { - 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); } }
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.maximets@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(-)