Re: [ovs-dev] [PATCH v6 0/9] dpif-netdev: Partial HWOL fixes/refactoring/unit-tests.
Ilya, I'm not yet familiar with the test syntax in .at file, but the rest of the series looks good. Regards, Asaf Penso > -Original Message- > From: Ilya Maximets > Sent: Wednesday, February 20, 2019 2:47 PM > To: ovs-dev@openvswitch.org; Ian Stokes > Cc: Flavio Leitner ; Ophir Munk > ; Kevin Traynor ; Roni Bar > Yanai ; Finn Christensen ; Asaf > Penso ; Ilya Maximets > Subject: [PATCH v6 0/9] dpif-netdev: Partial HWOL fixes/refactoring/unit- > tests. > > Few more fixes + dummy implementation to enable unit testing > of this feature. > > > Version 6: > * More specific log parsing in test. > * Added Acks from Flavio to corresponding patches. > > Version 5: > * 'dp_packet_mbuf_init' --> generic 'dp_packet_init_special' > * 'dp_packet_offload_invalidate' --> 'dp_packet_reset_offload' > * Offload bitmasks turned to enum. > * Dropped redundant OVS_UNUSED. > * Added Acks from Flavio to corresponding patches. > > Version 4: > * Rebase on current master. > > Version 3: > * Skip tests on non-Linux systems. > > Version 2: > * Patch #3 rebased on top of current master. > > > Ilya Maximets (9): > dpif-netdev: Reduce log level for not found mark id. > dp-packet: Constantify offloading APIs. > dp-packet: Refactor offloading API. > dp-packet: Add flow_mark support for non-DPDK case. > dp-packet: Copy flow mark on packet clone. > netdev-dummy: Implement dummy put/del flow offload API. > netdev-dummy: Set flow mark for offloaded flows. > netdev-dummy: Add flow offloading related logs. > dpif-netdev.at: Add basic test for partial HW offloading. > > lib/dp-packet.c | 19 ++-- > lib/dp-packet.h | 106 +++--- > lib/dpif-netdev.c| 4 +- > lib/netdev-dpdk.c| 6 +- > lib/netdev-dummy.c | 206 > +++ > lib/netdev.c | 4 +- > tests/dpif-netdev.at | 74 > 7 files changed, 337 insertions(+), 82 deletions(-) > > -- > 2.17.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-dpdk: Use single struct/union for flow offload items.
I see that now, thanks. I agree with you and I'm fine with this v2.
Regards,
Asaf Penso
> -Original Message-
> From: Ilya Maximets
> Sent: Tuesday, February 19, 2019 12:54 PM
> To: Asaf Penso ; ovs-dev@openvswitch.org; Ian
> Stokes
> Cc: Roni Bar Yanai ; Ophir Munk
>
> Subject: Re: [PATCH v2] netdev-dpdk: Use single struct/union for flow
> offload items.
>
> On 19.02.2019 13:45, Asaf Penso wrote:
> > Looks good, I would consider adding a default case for the switch with an
> appropriate message.
>
> There is an 'if' statement a few lines above the 'switch' that checks for
> unsupported protocols. And we should not print any warnings in case of
> unsupported protocol, if matching on it is not requested (i.e. all the fields
> wildcarded).
>
> >
> > Regards,
> > Asaf Penso
> >
> >> -Original Message-
> >> From: Ilya Maximets
> >> Sent: Wednesday, February 6, 2019 5:41 PM
> >> To: ovs-dev@openvswitch.org; Ian Stokes
> >> Cc: Asaf Penso ; Roni Bar Yanai
> >> ; Ophir Munk ; Ilya
> >> Maximets
> >> Subject: [PATCH v2] netdev-dpdk: Use single struct/union for flow
> >> offload items.
> >>
> >> Having a single structure allows to simplify the code path and clear
> >> all the items at once (probably faster). This does not increase stack
> >> memory usage because all the L4 related items grouped in a union.
> >>
> >> Changes:
> >> - Memsets combined.
> >> - 'ipv4_next_proto_mask' dropped as we already know the address
> >> and able to use 'mask.ipv4.hdr.next_proto_id' directly.
> >> - Group of 'if' statements for L4 protocols turned to a 'switch'.
> >> We can do that, because we don't have semi-local variables anymore.
> >> - Eliminated 'end_proto_check' label. Not needed with 'switch'.
> >>
> >> Additionally 'rte_memcpy' replaced with simple 'memcpy' as it makes
> >> no sense to use 'rte_memcpy' for 6 bytes.
> >>
> >> Signed-off-by: Ilya Maximets
> >> ---
> >>
> >> Version 2:
> >> * Dropped 'ipv4_next_proto_mask' pointer as we able to use
> >> 'mask.ipv4.hdr.next_proto_id' directly.
> >>
> >> lib/netdev-dpdk.c | 189
> >> +++---
> >> 1 file changed, 78 insertions(+), 111 deletions(-)
> >>
> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >> 26022a59c..d18dd1b6c 100644
> >> --- a/lib/netdev-dpdk.c
> >> +++ b/lib/netdev-dpdk.c
> >> @@ -4564,28 +4564,36 @@ netdev_dpdk_add_rte_flow_offload(struct
> >> netdev *netdev,
> >> struct flow_actions actions = { .actions = NULL, .cnt = 0 };
> >> struct rte_flow *flow;
> >> struct rte_flow_error error;
> >> -uint8_t *ipv4_next_proto_mask = NULL;
> >> +uint8_t proto = 0;
> >> int ret = 0;
> >> +struct flow_items {
> >> +struct rte_flow_item_eth eth;
> >> +struct rte_flow_item_vlan vlan;
> >> +struct rte_flow_item_ipv4 ipv4;
> >> +union {
> >> +struct rte_flow_item_tcp tcp;
> >> +struct rte_flow_item_udp udp;
> >> +struct rte_flow_item_sctp sctp;
> >> +struct rte_flow_item_icmp icmp;
> >> +};
> >> +} spec, mask;
> >> +
> >> +memset(, 0, sizeof spec);
> >> +memset(, 0, sizeof mask);
> >>
> >> /* Eth */
> >> -struct rte_flow_item_eth eth_spec;
> >> -struct rte_flow_item_eth eth_mask;
> >> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> >> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >> -memset(_spec, 0, sizeof eth_spec);
> >> -memset(_mask, 0, sizeof eth_mask);
> >> -rte_memcpy(_spec.dst, >flow.dl_dst, sizeof
> >> eth_spec.dst);
> >> -rte_memcpy(_spec.src, >flow.dl_src, sizeof
> eth_spec.src);
> >> -eth_spec.type = match->flow.dl_type;
> >> -
> >> -rte_memcpy(_mask.dst, >wc.masks.dl_dst,
> >> - sizeof eth_mask.dst);
> >> -rte_memcpy(_mask.src, >wc.masks.dl_src,
> >> - sizeof eth_mask.src);
> >> -eth_mask.type = match->wc.masks.dl_type;
> >> +memcpy(, >flow.dl_dst, sizeof spec.eth.dst);
> >> +memcpy(, >flow.dl_src, sizeof spec.eth.src);
&g
Re: [ovs-dev] [PATCH v2] netdev-dpdk: Use single struct/union for flow offload items.
Looks good, I would consider adding a default case for the switch with an
appropriate message.
Regards,
Asaf Penso
> -Original Message-
> From: Ilya Maximets
> Sent: Wednesday, February 6, 2019 5:41 PM
> To: ovs-dev@openvswitch.org; Ian Stokes
> Cc: Asaf Penso ; Roni Bar Yanai
> ; Ophir Munk ; Ilya
> Maximets
> Subject: [PATCH v2] netdev-dpdk: Use single struct/union for flow offload
> items.
>
> Having a single structure allows to simplify the code path and clear all the
> items at once (probably faster). This does not increase stack memory usage
> because all the L4 related items grouped in a union.
>
> Changes:
> - Memsets combined.
> - 'ipv4_next_proto_mask' dropped as we already know the address
> and able to use 'mask.ipv4.hdr.next_proto_id' directly.
> - Group of 'if' statements for L4 protocols turned to a 'switch'.
> We can do that, because we don't have semi-local variables anymore.
> - Eliminated 'end_proto_check' label. Not needed with 'switch'.
>
> Additionally 'rte_memcpy' replaced with simple 'memcpy' as it makes no
> sense to use 'rte_memcpy' for 6 bytes.
>
> Signed-off-by: Ilya Maximets
> ---
>
> Version 2:
> * Dropped 'ipv4_next_proto_mask' pointer as we able to use
> 'mask.ipv4.hdr.next_proto_id' directly.
>
> lib/netdev-dpdk.c | 189 +++---
> 1 file changed, 78 insertions(+), 111 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> 26022a59c..d18dd1b6c 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4564,28 +4564,36 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev *netdev,
> struct flow_actions actions = { .actions = NULL, .cnt = 0 };
> struct rte_flow *flow;
> struct rte_flow_error error;
> -uint8_t *ipv4_next_proto_mask = NULL;
> +uint8_t proto = 0;
> int ret = 0;
> +struct flow_items {
> +struct rte_flow_item_eth eth;
> +struct rte_flow_item_vlan vlan;
> +struct rte_flow_item_ipv4 ipv4;
> +union {
> +struct rte_flow_item_tcp tcp;
> +struct rte_flow_item_udp udp;
> +struct rte_flow_item_sctp sctp;
> +struct rte_flow_item_icmp icmp;
> +};
> +} spec, mask;
> +
> +memset(, 0, sizeof spec);
> +memset(, 0, sizeof mask);
>
> /* Eth */
> -struct rte_flow_item_eth eth_spec;
> -struct rte_flow_item_eth eth_mask;
> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> -memset(_spec, 0, sizeof eth_spec);
> -memset(_mask, 0, sizeof eth_mask);
> -rte_memcpy(_spec.dst, >flow.dl_dst, sizeof
> eth_spec.dst);
> -rte_memcpy(_spec.src, >flow.dl_src, sizeof eth_spec.src);
> -eth_spec.type = match->flow.dl_type;
> -
> -rte_memcpy(_mask.dst, >wc.masks.dl_dst,
> - sizeof eth_mask.dst);
> -rte_memcpy(_mask.src, >wc.masks.dl_src,
> - sizeof eth_mask.src);
> -eth_mask.type = match->wc.masks.dl_type;
> +memcpy(, >flow.dl_dst, sizeof spec.eth.dst);
> +memcpy(, >flow.dl_src, sizeof spec.eth.src);
> +spec.eth.type = match->flow.dl_type;
> +
> +memcpy(, >wc.masks.dl_dst, sizeof
> mask.eth.dst);
> +memcpy(, >wc.masks.dl_src, sizeof
> mask.eth.src);
> +mask.eth.type = match->wc.masks.dl_type;
>
> add_flow_pattern(, RTE_FLOW_ITEM_TYPE_ETH,
> - _spec, _mask);
> + , );
> } else {
> /*
> * If user specifies a flow (like UDP flow) without L2 patterns, @@ -
> 4598,50 +4606,37 @@ netdev_dpdk_add_rte_flow_offload(struct netdev
> *netdev,
> }
>
> /* VLAN */
> -struct rte_flow_item_vlan vlan_spec;
> -struct rte_flow_item_vlan vlan_mask;
> if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> -memset(_spec, 0, sizeof vlan_spec);
> -memset(_mask, 0, sizeof vlan_mask);
> -vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> -vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
> +spec.vlan.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> +mask.vlan.tci = match->wc.masks.vlans[0].tci &
> + ~htons(VLAN_CFI);
>
> /* match any protocols */
> -vlan_mask.inner_type = 0;
> +mask.vlan.inner_type = 0;
>
> add_flow_pattern(, RTE_FLOW_ITEM_TYPE_VLAN,
> - _spec, _mask);
> +
Re: [ovs-dev] [PATCH v4 3/9] dp-packet: Refactor offloading API.
Regards,
Asaf Penso
> -Original Message-
> From: Ilya Maximets
> Sent: Monday, February 18, 2019 2:10 PM
> To: Asaf Penso ; ovs-dev@openvswitch.org; Ian
> Stokes
> Cc: Roni Bar Yanai ; Flavio Leitner
>
> 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-boun...@openvswitch.org >> boun...@openvswitch.org> On Behalf Of Ilya Maximets
> >> Sent: Friday, February 15, 2019 3:07 PM
> >> To: ovs-dev@openvswitch.org; Ian Stokes
> >> Cc: Roni Bar Yanai ; Flavio Leitner
> >> ; Ilya Maximets
> >> 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
> >> ---
> >> 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(>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?
> &g
Re: [ovs-dev] [PATCH v4 4/9] dp-packet: Add flow_mark support for non-DPDK case.
Regards,
Asaf Penso
> -Original Message-
> From: ovs-dev-boun...@openvswitch.org boun...@openvswitch.org> On Behalf Of Flavio Leitner
> Sent: Friday, February 15, 2019 8:59 PM
> To: Ilya Maximets
> Cc: ovs-dev@openvswitch.org; Roni Bar Yanai
> Subject: Re: [ovs-dev] [PATCH v4 4/9] dp-packet: Add flow_mark support for
> non-DPDK case.
>
> On Fri, Feb 15, 2019 at 04:07:03PM +0300, Ilya Maximets wrote:
> > Additionally, new API call 'dp_packet_set_flow_mark' is needed for
> > packet clone. Mostly for dummy HWOL implementation.
> >
> > Signed-off-by: Ilya Maximets
> > ---
> > lib/dp-packet.h | 23 +--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/dp-packet.h b/lib/dp-packet.h index
> > 685edc90c..15c596a91 100644
> > --- a/lib/dp-packet.h
> > +++ b/lib/dp-packet.h
> > @@ -60,7 +60,9 @@ struct dp_packet {
> > 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?
> */
> > +#define DP_PACKET_OL_FLOW_MARK_MASK 0x2 /* Is the 'flow_mark'
> valid?
> > +*/
>
> Perhaps it's time for a enum?
>
> Otherwise it looks good to me.
> fbl
>
I also think that enum is a good approach.
>
> > uint32_t rss_hash; /* Packet hash. */
> > +uint32_t flow_mark; /* Packet flow mark. */
> > #endif
> > enum dp_packet_source source; /* Source of memory allocated as
> > 'base'. */
> >
> > @@ -556,6 +558,13 @@ dp_packet_has_flow_mark(const struct
> dp_packet *p, uint32_t *mark)
> > return false;
> > }
> >
> > +static inline void
> > +dp_packet_set_flow_mark(struct dp_packet *p, uint32_t mark) {
> > +p->mbuf.hash.fdir.hi = mark;
> > +p->mbuf.ol_flags |= PKT_RX_FDIR_ID; }
> > +
> > #else /* DPDK_NETDEV */
> > static inline void *
> > dp_packet_base(const struct dp_packet *b) @@ -657,11 +666,21 @@
> > dp_packet_l4_checksum_bad(const 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)
> > +dp_packet_has_flow_mark(const struct dp_packet *p, uint32_t *mark)
> > {
> > +if (p->ol_flags & DP_PACKET_OL_FLOW_MARK_MASK) {
> > +*mark = p->flow_mark;
> > +return true;
> > +}
> > return false;
> > }
> > +
> > +static inline void
> > +dp_packet_set_flow_mark(struct dp_packet *p, uint32_t mark) {
> > +p->flow_mark = mark;
> > +p->ol_flags |= DP_PACKET_OL_FLOW_MARK_MASK; }
> > #endif /* DPDK_NETDEV */
> >
> > static inline void
> > --
> > 2.17.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> i
> > l.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-
> devdata=02%7C01%7Cas
> >
> afp%40mellanox.com%7C82821a54567740211aaf08d69377a00d%7Ca652971c7
> d2e4d
> >
> 9ba6a4d149256f461b%7C0%7C0%7C636858539332346581sdata=mEyH3
> EnR9dIh
> > Rq5WxldGHJayVcnjp7RMdbqjiQXVaG8%3Dreserved=0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> il.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-
> devdata=02%7C01%7Casafp%40mellanox.com%7C82821a54567740211
> aaf08d69377a00d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63
> 6858539332356590sdata=HK%2FlvDuDociyZG7K79kbjOQ0TOKr%2Bmk
> MIyHvlYZCj%2BI%3Dreserved=0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 3/9] dp-packet: Refactor offloading API.
Regards,
Asaf Penso
> -Original Message-
> From: ovs-dev-boun...@openvswitch.org boun...@openvswitch.org> On Behalf Of Ilya Maximets
> Sent: Friday, February 15, 2019 3:07 PM
> To: ovs-dev@openvswitch.org; Ian Stokes
> Cc: Roni Bar Yanai ; Flavio Leitner
> ; Ilya Maximets
> 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
> ---
> 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(>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.nex
Re: [ovs-dev] [ovs-dev, v2] netdev-dpdk: Memset rte_flow_item on a need basis.
Regards,
Asaf Penso
> -Original Message-
> From: Ilya Maximets
> Sent: Tuesday, February 5, 2019 1:46 PM
> To: Asaf Penso ; ovs-dev@openvswitch.org
> Cc: Roni Bar Yanai ; Stokes, Ian
> ; Finn Christensen
> Subject: Re: [ovs-dev,v2] netdev-dpdk: Memset rte_flow_item on a need
> basis.
>
> On 04.02.2019 19:14, Asaf Penso wrote:
> > In netdev_dpdk_add_rte_flow_offload function different rte_flow_item
> > are created as part of the pattern matching.
> >
> > For most of them, there is a check whether the wildcards are not zero.
> > In case of zero, nothing is being done with the rte_flow_item.
> >
> > Befor the wildcard check, and regardless of the result, the
> > rte_flow_item is being memset.
> >
> > The patch moves the memset function only if the condition of the
> > wildcard is true, thus saving cycles of memset if not needed.
> >
> > Signed-off-by: Asaf Penso
> > ---
>
> I thought about this part of code a bit. IMHO, we could create a union from
> the L4 items and clear them all at once. Like this:
>
> uint8_t proto = 0;
> struct flow_items {
> struct rte_flow_item_eth eth;
> struct rte_flow_item_vlan vlan;
> struct rte_flow_item_ipv4 ipv4;
> union {
> struct rte_flow_item_tcp tcp;
> struct rte_flow_item_udp udp;
> struct rte_flow_item_sctp sctp;
> struct rte_flow_item_icmp icmp;
> };
> } spec, mask;
> uint8_t *ipv4_next_proto_mask = _proto_id;
>
> memset(, 0, sizeof spec);
> memset(, 0, sizeof mask);
>
>
> Ethernet is a common case, userspace datapath always has exact match on
> vlan,
> IPv4 is also the common case, I think. So current code in most cases we'll not
> call memset only for few of L4 protocols which are in the union here and
> does not eat extra memory.
> Anyway we're not on a very hot path.
>
> With that you'll be able to easily convert L4 proto checking 'if's to a single
> 'switch (proto)' statement and also clear the *ipv4_next_proto_mask
> unconditionally.
>
> What do you think ?
I fully agree and if you can have a separate patch it would be great.
>
> You could incorporate these changes to this patch or I could prepare the
> separate patch on top of it.
>
> Anyway, for the current version:
>
> Acked-by: Ilya Maximets
>
Thanks!
> > v2 update coding-style compliant for sizeof operator
> > ---
> > lib/netdev-dpdk.c | 36 ++--
> > 1 file changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 4bf0ca9..f07b10c 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -4570,18 +4570,18 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev *netdev,
> > /* Eth */
> > struct rte_flow_item_eth eth_spec;
> > struct rte_flow_item_eth eth_mask;
> > -memset(_spec, 0, sizeof(eth_spec));
> > -memset(_mask, 0, sizeof(eth_mask));
> > if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> > !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> > -rte_memcpy(_spec.dst, >flow.dl_dst,
> sizeof(eth_spec.dst));
> > -rte_memcpy(_spec.src, >flow.dl_src,
> sizeof(eth_spec.src));
> > +memset(_spec, 0, sizeof eth_spec);
> > +memset(_mask, 0, sizeof eth_mask);
> > +rte_memcpy(_spec.dst, >flow.dl_dst, sizeof
> eth_spec.dst);
> > +rte_memcpy(_spec.src, >flow.dl_src, sizeof
> > + eth_spec.src);
> > eth_spec.type = match->flow.dl_type;
> >
> > rte_memcpy(_mask.dst, >wc.masks.dl_dst,
> > - sizeof(eth_mask.dst));
> > + sizeof eth_mask.dst);
> > rte_memcpy(_mask.src, >wc.masks.dl_src,
> > - sizeof(eth_mask.src));
> > + sizeof eth_mask.src);
> > eth_mask.type = match->wc.masks.dl_type;
> >
> > add_flow_pattern(, RTE_FLOW_ITEM_TYPE_ETH, @@
> > -4600,9 +4600,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev
> *netdev,
> > /* VLAN */
> > struct rte_flow_item_vlan vlan_spec;
> > struct rte_flow_item_vlan vlan_mask;
> > -memset(_spec, 0, sizeof(vlan_spec));
> > -memset(_mask, 0, sizeof(vlan_mask));
> > if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> > +memset(_spec, 0, sizeof vlan_spec);
> > +memset(_mask, 0, sizeof vlan_mask);
> > vlan_spec.tci = match->fl
[ovs-dev] [PATCH v2] netdev-dpdk: Memset rte_flow_item on a need basis.
In netdev_dpdk_add_rte_flow_offload function different rte_flow_item are
created as part of the pattern matching.
For most of them, there is a check whether the wildcards are not zero.
In case of zero, nothing is being done with the rte_flow_item.
Befor the wildcard check, and regardless of the result, the
rte_flow_item is being memset.
The patch moves the memset function only if the condition of the
wildcard is true, thus saving cycles of memset if not needed.
Signed-off-by: Asaf Penso
---
v2 update coding-style compliant for sizeof operator
---
lib/netdev-dpdk.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 4bf0ca9..f07b10c 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -4570,18 +4570,18 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
/* Eth */
struct rte_flow_item_eth eth_spec;
struct rte_flow_item_eth eth_mask;
-memset(_spec, 0, sizeof(eth_spec));
-memset(_mask, 0, sizeof(eth_mask));
if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
!eth_addr_is_zero(match->wc.masks.dl_dst)) {
-rte_memcpy(_spec.dst, >flow.dl_dst, sizeof(eth_spec.dst));
-rte_memcpy(_spec.src, >flow.dl_src, sizeof(eth_spec.src));
+memset(_spec, 0, sizeof eth_spec);
+memset(_mask, 0, sizeof eth_mask);
+rte_memcpy(_spec.dst, >flow.dl_dst, sizeof eth_spec.dst);
+rte_memcpy(_spec.src, >flow.dl_src, sizeof eth_spec.src);
eth_spec.type = match->flow.dl_type;
rte_memcpy(_mask.dst, >wc.masks.dl_dst,
- sizeof(eth_mask.dst));
+ sizeof eth_mask.dst);
rte_memcpy(_mask.src, >wc.masks.dl_src,
- sizeof(eth_mask.src));
+ sizeof eth_mask.src);
eth_mask.type = match->wc.masks.dl_type;
add_flow_pattern(, RTE_FLOW_ITEM_TYPE_ETH,
@@ -4600,9 +4600,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
/* VLAN */
struct rte_flow_item_vlan vlan_spec;
struct rte_flow_item_vlan vlan_mask;
-memset(_spec, 0, sizeof(vlan_spec));
-memset(_mask, 0, sizeof(vlan_mask));
if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
+memset(_spec, 0, sizeof vlan_spec);
+memset(_mask, 0, sizeof vlan_mask);
vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
@@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
uint8_t proto = 0;
struct rte_flow_item_ipv4 ipv4_spec;
struct rte_flow_item_ipv4 ipv4_mask;
-memset(_spec, 0, sizeof(ipv4_spec));
-memset(_mask, 0, sizeof(ipv4_mask));
if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
+memset(_spec, 0, sizeof ipv4_spec);
+memset(_mask, 0, sizeof ipv4_mask);
ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
@@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_tcp tcp_spec;
struct rte_flow_item_tcp tcp_mask;
-memset(_spec, 0, sizeof(tcp_spec));
-memset(_mask, 0, sizeof(tcp_mask));
if (proto == IPPROTO_TCP) {
+memset(_spec, 0, sizeof tcp_spec);
+memset(_mask, 0, sizeof tcp_mask);
tcp_spec.hdr.src_port = match->flow.tp_src;
tcp_spec.hdr.dst_port = match->flow.tp_dst;
tcp_spec.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
@@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_udp udp_spec;
struct rte_flow_item_udp udp_mask;
-memset(_spec, 0, sizeof(udp_spec));
-memset(_mask, 0, sizeof(udp_mask));
if (proto == IPPROTO_UDP) {
+memset(_spec, 0, sizeof udp_spec);
+memset(_mask, 0, sizeof udp_mask);
udp_spec.hdr.src_port = match->flow.tp_src;
udp_spec.hdr.dst_port = match->flow.tp_dst;
@@ -4708,9 +4708,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_sctp sctp_spec;
struct rte_flow_item_sctp sctp_mask;
-memset(_spec, 0, sizeof(sctp_spec));
-memset(_mask, 0, sizeof(sctp_mask));
if (proto == IPPROTO_SCTP) {
+memset(_spec, 0, sizeof sctp_spec);
+memset(_mask, 0, sizeof sctp_mask);
sctp_spec.hdr.src_port = match->flow.tp_src;
sctp_spec.hdr.dst_port = match->flow.tp_dst;
@@ -4729,9 +4729,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_icmp icmp_spec;
struct rte_flow_item_icmp icmp_mask;
-memset(_spec, 0, sizeof(icmp_spec));
-memset(_mask, 0, sizeof(icmp_mask));
if (proto == IPPROTO_ICMP) {
+memset(_spec, 0, sizeof icmp_spec);
+memset(_ma
Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
Regards,
Asaf Penso
> -Original Message-
> From: Ilya Maximets
> Sent: Monday, February 4, 2019 5:41 PM
> To: Asaf Penso ; ovs-dev@openvswitch.org
> Cc: Roni Bar Yanai ; Stokes, Ian
>
> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
>
> On 04.02.2019 18:08, Asaf Penso wrote:
> >
> >
> > Regards,
> > Asaf Penso
> >
> >> -Original Message-
> >> From: Ilya Maximets
> >> Sent: Monday, February 4, 2019 4:14 PM
> >> To: Asaf Penso ; ovs-dev@openvswitch.org
> >> Cc: Roni Bar Yanai ; Stokes, Ian
> >>
> >> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need
> basis.
> >>
> >> On 04.02.2019 15:50, Asaf Penso wrote:
> >>> In netdev_dpdk_add_rte_flow_offload function different
> rte_flow_item
> >>> are created as part of the pattern matching.
> >>>
> >>> For most of them, there is a check whether the wildcards are not zero.
> >>> In case of zero, nothing is being done with the rte_flow_item.
> >>>
> >>> Befor the wildcard check, and regardless of the result, the
> >>> rte_flow_item is being memset.
> >>>
> >>> The patch moves the memset function only if the condition of the
> >>> wildcard is true, thus saving cycles of memset if not needed.
> >>
> >> Structures are actually used only inside their 'if' blocks.
> >> IMHO, it's better to move the definitions inside too.
> >>
> >
> > Inside each 'if' block there is a call to add_flow_pattern that updates the
> pattern's pointers to these structs.
> > Having them defined inside the block will cause their value to be corrupted
> once we exit the block.
> > They are needed all the way until rte_flow_create call.
>
> Oh. Sorry. You're right. I forget about that.
>
> BTW, as you touching this code, maybe you could make it a bit more coding-
> style compliant ? It's about sizeof operator. Coding-style asks to not
> parenthesize the operand. Like this:
>
> memset(_spec, 0, sizeof eth_spec);
>
> It'll be nice if you could fix that.
> You may also fix that for the 'rte_memcpy' in the fist 'if'.
Sure, no issue! I'll fix it for all the places in my patch, both in memset and
in rte_memcpy.
Anything else or can I upload v2?
>
> >
> >
> >>>
> >>> Signed-off-by: Asaf Penso
> >>> ---
> >>> lib/netdev-dpdk.c | 28 ++--
> >>> 1 file changed, 14 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >>> 4bf0ca9..5216b74 100644
> >>> --- a/lib/netdev-dpdk.c
> >>> +++ b/lib/netdev-dpdk.c
> >>> @@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct
> >> netdev *netdev,
> >>> /* Eth */
> >>> struct rte_flow_item_eth eth_spec;
> >>> struct rte_flow_item_eth eth_mask;
> >>> -memset(_spec, 0, sizeof(eth_spec));
> >>> -memset(_mask, 0, sizeof(eth_mask));
> >>> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> >>> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >>> +memset(_spec, 0, sizeof(eth_spec));
> >>> +memset(_mask, 0, sizeof(eth_mask));
> >>> rte_memcpy(_spec.dst, >flow.dl_dst,
> >> sizeof(eth_spec.dst));
> >>> rte_memcpy(_spec.src, >flow.dl_src,
> >> sizeof(eth_spec.src));
> >>> eth_spec.type = match->flow.dl_type; @@ -4600,9 +4600,9 @@
> >>> netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> >>> /* VLAN */
> >>> struct rte_flow_item_vlan vlan_spec;
> >>> struct rte_flow_item_vlan vlan_mask;
> >>> -memset(_spec, 0, sizeof(vlan_spec));
> >>> -memset(_mask, 0, sizeof(vlan_mask));
> >>> if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> >>> +memset(_spec, 0, sizeof(vlan_spec));
> >>> +memset(_mask, 0, sizeof(vlan_mask));
> >>> vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> >>> vlan_mask.tci = match->wc.masks.vlans[0].tci &
> >>> ~htons(VLAN_CFI);
> >>>
> >>> @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> >> netdev *netdev,
> >>> uint8_t proto = 0;
> >>> struct rte_flow_item_ipv4 ipv4_spe
Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
Regards,
Asaf Penso
> -Original Message-
> From: Ilya Maximets
> Sent: Monday, February 4, 2019 4:14 PM
> To: Asaf Penso ; ovs-dev@openvswitch.org
> Cc: Roni Bar Yanai ; Stokes, Ian
>
> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
>
> On 04.02.2019 15:50, Asaf Penso wrote:
> > In netdev_dpdk_add_rte_flow_offload function different rte_flow_item
> > are created as part of the pattern matching.
> >
> > For most of them, there is a check whether the wildcards are not zero.
> > In case of zero, nothing is being done with the rte_flow_item.
> >
> > Befor the wildcard check, and regardless of the result, the
> > rte_flow_item is being memset.
> >
> > The patch moves the memset function only if the condition of the
> > wildcard is true, thus saving cycles of memset if not needed.
>
> Structures are actually used only inside their 'if' blocks.
> IMHO, it's better to move the definitions inside too.
>
Inside each 'if' block there is a call to add_flow_pattern that updates the
pattern's pointers to these structs.
Having them defined inside the block will cause their value to be corrupted
once we exit the block.
They are needed all the way until rte_flow_create call.
> >
> > Signed-off-by: Asaf Penso
> > ---
> > lib/netdev-dpdk.c | 28 ++--
> > 1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 4bf0ca9..5216b74 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev *netdev,
> > /* Eth */
> > struct rte_flow_item_eth eth_spec;
> > struct rte_flow_item_eth eth_mask;
> > -memset(_spec, 0, sizeof(eth_spec));
> > -memset(_mask, 0, sizeof(eth_mask));
> > if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> > !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> > +memset(_spec, 0, sizeof(eth_spec));
> > +memset(_mask, 0, sizeof(eth_mask));
> > rte_memcpy(_spec.dst, >flow.dl_dst,
> sizeof(eth_spec.dst));
> > rte_memcpy(_spec.src, >flow.dl_src,
> sizeof(eth_spec.src));
> > eth_spec.type = match->flow.dl_type; @@ -4600,9 +4600,9 @@
> > netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> > /* VLAN */
> > struct rte_flow_item_vlan vlan_spec;
> > struct rte_flow_item_vlan vlan_mask;
> > -memset(_spec, 0, sizeof(vlan_spec));
> > -memset(_mask, 0, sizeof(vlan_mask));
> > if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> > +memset(_spec, 0, sizeof(vlan_spec));
> > +memset(_mask, 0, sizeof(vlan_mask));
> > vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> > vlan_mask.tci = match->wc.masks.vlans[0].tci &
> > ~htons(VLAN_CFI);
> >
> > @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev *netdev,
> > uint8_t proto = 0;
> > struct rte_flow_item_ipv4 ipv4_spec;
> > struct rte_flow_item_ipv4 ipv4_mask;
> > -memset(_spec, 0, sizeof(ipv4_spec));
> > -memset(_mask, 0, sizeof(ipv4_mask));
> > if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> > +memset(_spec, 0, sizeof(ipv4_spec));
> > +memset(_mask, 0, sizeof(ipv4_mask));
> >
> > ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
> > ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
> > @@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev
> > *netdev,
> >
> > struct rte_flow_item_tcp tcp_spec;
> > struct rte_flow_item_tcp tcp_mask;
> > -memset(_spec, 0, sizeof(tcp_spec));
> > -memset(_mask, 0, sizeof(tcp_mask));
> > if (proto == IPPROTO_TCP) {
> > +memset(_spec, 0, sizeof(tcp_spec));
> > +memset(_mask, 0, sizeof(tcp_mask));
> > tcp_spec.hdr.src_port = match->flow.tp_src;
> > tcp_spec.hdr.dst_port = match->flow.tp_dst;
> > tcp_spec.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
> > @@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev
> > *netdev,
> >
> > struct rte_flow_item_udp udp_spec;
> > struct rte_flow_item_udp udp_mask;
> > -memset(_spec, 0, sizeof(udp_spec));
> > -memset(_mask, 0, sizeof(udp_mask));
> > if (proto == IPPROTO_UDP) {
> > +mem
[ovs-dev] [PATCH] netdev-dpdk: Memset rte_flow_item on a need basis.
In netdev_dpdk_add_rte_flow_offload function different rte_flow_item are
created as part of the pattern matching.
For most of them, there is a check whether the wildcards are not zero.
In case of zero, nothing is being done with the rte_flow_item.
Befor the wildcard check, and regardless of the result, the
rte_flow_item is being memset.
The patch moves the memset function only if the condition of the
wildcard is true, thus saving cycles of memset if not needed.
Signed-off-by: Asaf Penso
---
lib/netdev-dpdk.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 4bf0ca9..5216b74 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
/* Eth */
struct rte_flow_item_eth eth_spec;
struct rte_flow_item_eth eth_mask;
-memset(_spec, 0, sizeof(eth_spec));
-memset(_mask, 0, sizeof(eth_mask));
if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
!eth_addr_is_zero(match->wc.masks.dl_dst)) {
+memset(_spec, 0, sizeof(eth_spec));
+memset(_mask, 0, sizeof(eth_mask));
rte_memcpy(_spec.dst, >flow.dl_dst, sizeof(eth_spec.dst));
rte_memcpy(_spec.src, >flow.dl_src, sizeof(eth_spec.src));
eth_spec.type = match->flow.dl_type;
@@ -4600,9 +4600,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
/* VLAN */
struct rte_flow_item_vlan vlan_spec;
struct rte_flow_item_vlan vlan_mask;
-memset(_spec, 0, sizeof(vlan_spec));
-memset(_mask, 0, sizeof(vlan_mask));
if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
+memset(_spec, 0, sizeof(vlan_spec));
+memset(_mask, 0, sizeof(vlan_mask));
vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
@@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
uint8_t proto = 0;
struct rte_flow_item_ipv4 ipv4_spec;
struct rte_flow_item_ipv4 ipv4_mask;
-memset(_spec, 0, sizeof(ipv4_spec));
-memset(_mask, 0, sizeof(ipv4_mask));
if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
+memset(_spec, 0, sizeof(ipv4_spec));
+memset(_mask, 0, sizeof(ipv4_mask));
ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
ipv4_spec.hdr.time_to_live= match->flow.nw_ttl;
@@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_tcp tcp_spec;
struct rte_flow_item_tcp tcp_mask;
-memset(_spec, 0, sizeof(tcp_spec));
-memset(_mask, 0, sizeof(tcp_mask));
if (proto == IPPROTO_TCP) {
+memset(_spec, 0, sizeof(tcp_spec));
+memset(_mask, 0, sizeof(tcp_mask));
tcp_spec.hdr.src_port = match->flow.tp_src;
tcp_spec.hdr.dst_port = match->flow.tp_dst;
tcp_spec.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
@@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_udp udp_spec;
struct rte_flow_item_udp udp_mask;
-memset(_spec, 0, sizeof(udp_spec));
-memset(_mask, 0, sizeof(udp_mask));
if (proto == IPPROTO_UDP) {
+memset(_spec, 0, sizeof(udp_spec));
+memset(_mask, 0, sizeof(udp_mask));
udp_spec.hdr.src_port = match->flow.tp_src;
udp_spec.hdr.dst_port = match->flow.tp_dst;
@@ -4708,9 +4708,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_sctp sctp_spec;
struct rte_flow_item_sctp sctp_mask;
-memset(_spec, 0, sizeof(sctp_spec));
-memset(_mask, 0, sizeof(sctp_mask));
if (proto == IPPROTO_SCTP) {
+memset(_spec, 0, sizeof(sctp_spec));
+memset(_mask, 0, sizeof(sctp_mask));
sctp_spec.hdr.src_port = match->flow.tp_src;
sctp_spec.hdr.dst_port = match->flow.tp_dst;
@@ -4729,9 +4729,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
struct rte_flow_item_icmp icmp_spec;
struct rte_flow_item_icmp icmp_mask;
-memset(_spec, 0, sizeof(icmp_spec));
-memset(_mask, 0, sizeof(icmp_mask));
if (proto == IPPROTO_ICMP) {
+memset(_spec, 0, sizeof(icmp_spec));
+memset(_mask, 0, sizeof(icmp_mask));
icmp_spec.hdr.icmp_type = (uint8_t)ntohs(match->flow.tp_src);
icmp_spec.hdr.icmp_code = (uint8_t)ntohs(match->flow.tp_dst);
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
