Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Hi Eelco ,
Replies inline.
> -Original Message-
> From: Eelco Chaudron
> Sent: Wednesday, June 30, 2021 3:17 PM
> To: Amber, Kumar ; Van Haaren, Harry
>
> Cc: d...@openvswitch.org; i.maxim...@ovn.org; Flavio Leitner
>
> Subject: Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and
> function pointer for miniflow extract
>
>
>
> On 17 Jun 2021, at 18:27, Kumar Amber wrote:
>
> > This patch introduces the mfex function pointers which allows the user
> > to switch between different miniflow extract implementations which are
> > provided by the OVS based on optimized ISA CPU.
> >
> > The user can query for the available minflow extract variants
> > available for that CPU by following commands:
> >
> > $ovs-appctl dpif-netdev/miniflow-parser-get
> >
> > Similarly an user can set the miniflow implementation by the following
> > command :
> >
> > $ ovs-appctl dpif-netdev/miniflow-parser-set name
> >
> > This allow for more performance and flexibility to the user to choose
> > the miniflow implementation according to the needs.
> >
> > Signed-off-by: Kumar Amber
> > Co-authored-by: Harry van Haaren
> > Signed-off-by: Harry van Haaren
> > ---
> > lib/automake.mk | 2 +
> > lib/dpif-netdev-avx512.c | 32 ++--
> > lib/dpif-netdev-private-extract.c | 86
> > lib/dpif-netdev-private-extract.h | 94 ++
> > lib/dpif-netdev-private-thread.h | 4 +
> > lib/dpif-netdev.c | 126 +-
> > 6 files changed, 337 insertions(+), 7 deletions(-) create mode
> > 100644 lib/dpif-netdev-private-extract.c create mode 100644
> > lib/dpif-netdev-private-extract.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk index
> > 49f42c2a3..6657b9ae5 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> > lib/dpif-netdev-private-dpcls.h \
> > lib/dpif-netdev-private-dpif.c \
> > lib/dpif-netdev-private-dpif.h \
> > + lib/dpif-netdev-private-extract.c \
> > + lib/dpif-netdev-private-extract.h \
> > lib/dpif-netdev-private-flow.h \
> > lib/dpif-netdev-private-hwol.h \
> > lib/dpif-netdev-private-thread.h \
> > diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c index
> > f9b199637..bb99b23ff 100644
> > --- a/lib/dpif-netdev-avx512.c
> > +++ b/lib/dpif-netdev-avx512.c
> > @@ -148,6 +148,15 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> > * // do all processing (HWOL->MFEX->EMC->SMC)
> > * }
> > */
> > +
> > +/* Do a batch minfilow extract into keys. */
> > +uint32_t mf_mask = 0;
> > +if (pmd->miniflow_extract_opt) {
> > +mf_mask = pmd->miniflow_extract_opt(packets, keys,
> > +batch_size, in_port,
> > +(void *) pmd);
> > +}
> > +/* Perform first packet interation */
> > uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
> > uint32_t iter = lookup_pkts_bitmask;
> > while (iter) {
> > @@ -159,6 +168,12 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> > pkt_metadata_init(>md, in_port);
> >
> > struct dp_netdev_flow *f = NULL;
> > +struct netdev_flow_key *key = [i];
> > +
> > +/* Check the minfiflow mask to see if the packet was correctly
> > +* classifed by vector mfex else do a scalar miniflow extract
> > +* for that packet. */
> > +uint32_t mfex_hit = (mf_mask & (1 << i));
> >
> > /* Check for partial hardware offload mark. */
> > uint32_t mark;
> > @@ -166,7 +181,13 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> > f = mark_to_flow_find(pmd, mark);
> > if (f) {
> > rules[i] = >cr;
> > -pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> > +/* If AVX512 MFEX already classified the packet, use it. */
> > +if (mfex_hit) {
> > +pkt_meta[i].tcp_flags =
> > miniflow_get_tcp_flags(>mf);
> > +} else {
> > +pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> > +}
> > +
> > pkt_meta[i].bytes = dp_packe
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On 17 Jun 2021, at 18:27, Kumar Amber wrote:
> This patch introduces the mfex function pointers which allows
> the user to switch between different miniflow extract implementations
> which are provided by the OVS based on optimized ISA CPU.
>
> The user can query for the available minflow extract variants available
> for that CPU by following commands:
>
> $ovs-appctl dpif-netdev/miniflow-parser-get
>
> Similarly an user can set the miniflow implementation by the following
> command :
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set name
>
> This allow for more performance and flexibility to the user to choose
> the miniflow implementation according to the needs.
>
> Signed-off-by: Kumar Amber
> Co-authored-by: Harry van Haaren
> Signed-off-by: Harry van Haaren
> ---
> lib/automake.mk | 2 +
> lib/dpif-netdev-avx512.c | 32 ++--
> lib/dpif-netdev-private-extract.c | 86
> lib/dpif-netdev-private-extract.h | 94 ++
> lib/dpif-netdev-private-thread.h | 4 +
> lib/dpif-netdev.c | 126 +-
> 6 files changed, 337 insertions(+), 7 deletions(-)
> create mode 100644 lib/dpif-netdev-private-extract.c
> create mode 100644 lib/dpif-netdev-private-extract.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 49f42c2a3..6657b9ae5 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> lib/dpif-netdev-private-dpcls.h \
> lib/dpif-netdev-private-dpif.c \
> lib/dpif-netdev-private-dpif.h \
> + lib/dpif-netdev-private-extract.c \
> + lib/dpif-netdev-private-extract.h \
> lib/dpif-netdev-private-flow.h \
> lib/dpif-netdev-private-hwol.h \
> lib/dpif-netdev-private-thread.h \
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index f9b199637..bb99b23ff 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -148,6 +148,15 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> * // do all processing (HWOL->MFEX->EMC->SMC)
> * }
> */
> +
> +/* Do a batch minfilow extract into keys. */
> +uint32_t mf_mask = 0;
> +if (pmd->miniflow_extract_opt) {
> +mf_mask = pmd->miniflow_extract_opt(packets, keys,
> +batch_size, in_port,
> +(void *) pmd);
> +}
> +/* Perform first packet interation */
> uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
> uint32_t iter = lookup_pkts_bitmask;
> while (iter) {
> @@ -159,6 +168,12 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> pkt_metadata_init(>md, in_port);
>
> struct dp_netdev_flow *f = NULL;
> +struct netdev_flow_key *key = [i];
> +
> +/* Check the minfiflow mask to see if the packet was correctly
> +* classifed by vector mfex else do a scalar miniflow extract
> +* for that packet. */
> +uint32_t mfex_hit = (mf_mask & (1 << i));
>
> /* Check for partial hardware offload mark. */
> uint32_t mark;
> @@ -166,7 +181,13 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> f = mark_to_flow_find(pmd, mark);
> if (f) {
> rules[i] = >cr;
> -pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> +/* If AVX512 MFEX already classified the packet, use it. */
> +if (mfex_hit) {
> +pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
> +} else {
> +pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> +}
> +
> pkt_meta[i].bytes = dp_packet_size(packet);
> phwol_hits++;
> hwol_emc_smc_hitmask |= (1 << i);
> @@ -174,11 +195,12 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> }
> }
>
> -/* Do miniflow extract into keys. */
> -struct netdev_flow_key *key = [i];
> -miniflow_extract(packet, >mf);
> +if (!mfex_hit) {
> +/* Do a scalar miniflow extract into keys */
> +miniflow_extract(packet, >mf);
> +}
>
> -/* Cache TCP and byte values for all packets. */
> +/* Cache TCP and byte values for all packets */
> pkt_meta[i].bytes = dp_packet_size(packet);
> pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
>
> diff --git a/lib/dpif-netdev-private-extract.c
> b/lib/dpif-netdev-private-extract.c
> new file mode 100644
> index 0..fcc56ef26
> --- /dev/null
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -0,0 +1,86 @@
> +/*
> + * Copyright (c) 2021 Intel.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + *
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On 29 Jun 2021, at 17:23, Van Haaren, Harry wrote:
>> -Original Message-
>> From: Eelco Chaudron
>> Sent: Tuesday, June 29, 2021 2:56 PM
>> To: Amber, Kumar
>> Cc: Van Haaren, Harry ; d...@openvswitch.org;
>> i.maxim...@ovn.org; Flavio Leitner
>> Subject: Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function
>> pointer for miniflow extract
>>
>>
>>
>> On 29 Jun 2021, at 13:59, Amber, Kumar wrote:
>>
>>> Hi Eelco,
>>>
>>> Thanks a lot for the comments and my replies are inline.
>>>
>>
>>
>>
>>>>> +return;
>>>>> +}
>>>>> +
>>>>> +/* Add all mfex functions to reply string. */
>>>>> +struct ds reply = DS_EMPTY_INITIALIZER;
>>>>> +ds_put_cstr(, "Available Optimized Miniflow Extracts:\n");
>>>>> +for (uint32_t i = 0; i < count; i++) {
>>>>> +ds_put_format(, " %s (available: %s)\n",
>>>>> + mfex_impls[i].name, mfex_impls[i].available ?
>>>>> + "True" : "False");
>>>>> +}
>>>>> +unixctl_command_reply(conn, ds_cstr());
>>>>> +ds_destroy();
>>>>
>>>> I think this command must output the currently configured values for all
>>>> data paths, or else there is no easy way to see the current setting.
>>>>
>>>
>>> We are planning to do a separate patch for implementing the same for DPIF,
>>> MFEX adnd DPCLS.
>>>
>>
>> If you do, please do it ASAP, as I think this feature should not get in
>> without being
>> able to see in the field what the actual configuration is.
>
> Hi Eelco,
>
> OK it seems that there's a lot of focus around visibility of implementation
> used here.
> That's good and makes sense, lets focus to get that improved.
>
> So moving forward, how about the below output for each command?
> (Note, I had a quick chat with Amber & Cian over IM here to get to the below!)
>
> The mapping is not always very obvious, as e.g. DPCLS ports can be
> re-assigned between PMD threads.
> (Note the implementation of DPCLS might be a bit tricky, as specialized
> subtable searches
> aren't externally exposed. I'm confident we'll find a solution.)
>
> DPIF and MFEX are enabled per-PMD thread, and are always consistent for all
> datapath threads.
Not sure what you meant the the “always consistent for all datapath threads”?
If you mean all PMDs from the same data-path have the same MFEX function, this
is not true once you run the study mode.
>
> Today's commands have very similar output, now with (name: value) data points
> added.
> Example for DPIF: (pmds: 15,16) means pmd threads 15 and 16 are running
> that impl.
>
> Thoughts on the below commands, and added info? Regards, -Harry
I think the output for the additional PMDs is what we need. I’ve never looked
at the DPCL implementation, so no idea if the number of ports is enough, or we
need the explicit ports, etc.? I’ll let others who reviewed that reply ;)
Thanks Harry for digging into this!
>
> $ ovs-appctl dpif-netdev/subtable-lookup-prio-get
> Available lookup functions (priority : name)
> 0 : autovalidator (ports: none)
> 1 : generic (ports: none)
> 3 : avx512_gather (ports: 2) # number of DPCLS ports using this impl
>
> $ ovs-appctl dpif-netdev/dpif-set-impl
> Available DPIF impls:
> dpif_scalar (pmds: 15,16)# PMD thread ids using this DPIF impl
> dpif_avx512 (pmds: none)
>
> $ ovs-appctl dpif-netdev/miniflow-parser-get
> Available Optimized Miniflow Extracts:
> autovalidator (available: True, pmds: none)
> disable (available: True, pmds: none)
> study (available: True, pmds: none)
> avx512_vbmi_ipv4_udp (available: True, pmds: 15,16) # PMD thread
> ids using this MFEX impl
> .
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
> -Original Message-
> From: Eelco Chaudron
> Sent: Tuesday, June 29, 2021 2:56 PM
> To: Amber, Kumar
> Cc: Van Haaren, Harry ; d...@openvswitch.org;
> i.maxim...@ovn.org; Flavio Leitner
> Subject: Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function
> pointer for miniflow extract
>
>
>
> On 29 Jun 2021, at 13:59, Amber, Kumar wrote:
>
> > Hi Eelco,
> >
> > Thanks a lot for the comments and my replies are inline.
> >
>
>
>
> >>> +return;
> >>> +}
> >>> +
> >>> +/* Add all mfex functions to reply string. */
> >>> +struct ds reply = DS_EMPTY_INITIALIZER;
> >>> +ds_put_cstr(, "Available Optimized Miniflow Extracts:\n");
> >>> +for (uint32_t i = 0; i < count; i++) {
> >>> +ds_put_format(, " %s (available: %s)\n",
> >>> + mfex_impls[i].name, mfex_impls[i].available ?
> >>> + "True" : "False");
> >>> +}
> >>> +unixctl_command_reply(conn, ds_cstr());
> >>> +ds_destroy();
> >>
> >> I think this command must output the currently configured values for all
> >> data paths, or else there is no easy way to see the current setting.
> >>
> >
> > We are planning to do a separate patch for implementing the same for DPIF,
> > MFEX adnd DPCLS.
> >
>
> If you do, please do it ASAP, as I think this feature should not get in
> without being
> able to see in the field what the actual configuration is.
Hi Eelco,
OK it seems that there's a lot of focus around visibility of implementation
used here.
That's good and makes sense, lets focus to get that improved.
So moving forward, how about the below output for each command?
(Note, I had a quick chat with Amber & Cian over IM here to get to the below!)
The mapping is not always very obvious, as e.g. DPCLS ports can be re-assigned
between PMD threads.
(Note the implementation of DPCLS might be a bit tricky, as specialized
subtable searches
aren't externally exposed. I'm confident we'll find a solution.)
DPIF and MFEX are enabled per-PMD thread, and are always consistent for all
datapath threads.
Today's commands have very similar output, now with (name: value) data points
added.
Example for DPIF: (pmds: 15,16) means pmd threads 15 and 16 are running that
impl.
Thoughts on the below commands, and added info? Regards, -Harry
$ ovs-appctl dpif-netdev/subtable-lookup-prio-get
Available lookup functions (priority : name)
0 : autovalidator (ports: none)
1 : generic (ports: none)
3 : avx512_gather (ports: 2) # number of DPCLS ports using this impl
$ ovs-appctl dpif-netdev/dpif-set-impl
Available DPIF impls:
dpif_scalar (pmds: 15,16)# PMD thread ids using this DPIF impl
dpif_avx512 (pmds: none)
$ ovs-appctl dpif-netdev/miniflow-parser-get
Available Optimized Miniflow Extracts:
autovalidator (available: True, pmds: none)
disable (available: True, pmds: none)
study (available: True, pmds: none)
avx512_vbmi_ipv4_udp (available: True, pmds: 15,16) # PMD thread ids
using this MFEX impl
.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On 29 Jun 2021, at 13:59, Amber, Kumar wrote:
> Hi Eelco,
>
> Thanks a lot for the comments and my replies are inline.
>
>>> +return;
>>> +}
>>> +
>>> +/* Add all mfex functions to reply string. */
>>> +struct ds reply = DS_EMPTY_INITIALIZER;
>>> +ds_put_cstr(, "Available Optimized Miniflow Extracts:\n");
>>> +for (uint32_t i = 0; i < count; i++) {
>>> +ds_put_format(, " %s (available: %s)\n",
>>> + mfex_impls[i].name, mfex_impls[i].available ?
>>> + "True" : "False");
>>> +}
>>> +unixctl_command_reply(conn, ds_cstr());
>>> +ds_destroy();
>>
>> I think this command must output the currently configured values for all
>> data paths, or else there is no easy way to see the current setting.
>>
>
> We are planning to do a separate patch for implementing the same for DPIF,
> MFEX adnd DPCLS.
>
If you do, please do it ASAP, as I think this feature should not get in without
being able to see in the field what the actual configuration is.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Hi Eelco,
Thanks a lot for the comments and my replies are inline.
> -Original Message-
> From: Eelco Chaudron
> Sent: Tuesday, June 29, 2021 2:59 PM
> To: Amber, Kumar ; Van Haaren, Harry
>
> Cc: d...@openvswitch.org; i.maxim...@ovn.org; Flavio Leitner
>
> Subject: Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and
> function pointer for miniflow extract
>
> Hi Kumar/Harry,
>
> Find my comments inline.
>
> //Eelco
>
>
> On 17 Jun 2021, at 18:27, Kumar Amber wrote:
>
> > This patch introduces the mfex function pointers which allows the user
> > to switch between different miniflow extract implementations which are
> > provided by the OVS based on optimized ISA CPU.
> >
> > The user can query for the available minflow extract variants
> > available for that CPU by following commands:
> >
> > $ovs-appctl dpif-netdev/miniflow-parser-get
> >
> > Similarly an user can set the miniflow implementation by the following
> > command :
> >
> > $ ovs-appctl dpif-netdev/miniflow-parser-set name
> >
> > This allow for more performance and flexibility to the user to choose
> > the miniflow implementation according to the needs.
> >
> > Signed-off-by: Kumar Amber
> > Co-authored-by: Harry van Haaren
> > Signed-off-by: Harry van Haaren
> > ---
> > lib/automake.mk | 2 +
> > lib/dpif-netdev-avx512.c | 32 ++--
> > lib/dpif-netdev-private-extract.c | 86
> > lib/dpif-netdev-private-extract.h | 94 ++
> > lib/dpif-netdev-private-thread.h | 4 +
> > lib/dpif-netdev.c | 126 +-
> > 6 files changed, 337 insertions(+), 7 deletions(-) create mode
> > 100644 lib/dpif-netdev-private-extract.c create mode 100644
> > lib/dpif-netdev-private-extract.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk index
> > 49f42c2a3..6657b9ae5 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> > lib/dpif-netdev-private-dpcls.h \
> > lib/dpif-netdev-private-dpif.c \
> > lib/dpif-netdev-private-dpif.h \
> > + lib/dpif-netdev-private-extract.c \
> > + lib/dpif-netdev-private-extract.h \
> > lib/dpif-netdev-private-flow.h \
> > lib/dpif-netdev-private-hwol.h \
> > lib/dpif-netdev-private-thread.h \
> > diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c index
> > f9b199637..bb99b23ff 100644
> > --- a/lib/dpif-netdev-avx512.c
> > +++ b/lib/dpif-netdev-avx512.c
> > @@ -148,6 +148,15 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> > * // do all processing (HWOL->MFEX->EMC->SMC)
> > * }
> > */
> > +
> > +/* Do a batch minfilow extract into keys. */
> > +uint32_t mf_mask = 0;
> > +if (pmd->miniflow_extract_opt) {
>
> This will need some atomic get/use, or else we will crash here on change.
Changed to following in v5.
miniflow_extract_func mfex_func;
atomic_read_relaxed(>miniflow_extract_opt, _func);
if (mfex_func) {
>
> > +mf_mask = pmd->miniflow_extract_opt(packets, keys,
> > +batch_size, in_port,
> > +(void *) pmd);
>
> Don’t think we should cast to void here, but I guess the callback should take
> struct dp_netdev_pmd_thread?
>
Fixed in v5 already.
>
> > +}
> > +/* Perform first packet interation */
> > uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
> > uint32_t iter = lookup_pkts_bitmask;
> > while (iter) {
> > @@ -159,6 +168,12 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> > pkt_metadata_init(>md, in_port);
> >
> > struct dp_netdev_flow *f = NULL;
> > +struct netdev_flow_key *key = [i];
> > +
> > +/* Check the minfiflow mask to see if the packet was correctly
> > +* classifed by vector mfex else do a scalar miniflow extract
> > +* for that packet. */
> > +uint32_t mfex_hit = (mf_mask & (1 << i));
> >
> > /* Check for partial hardware offload mark. */
> > uint32_t mark;
> > @@ -166,7 +181,13 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> > f = mark_to_flow_find(pmd, mark);
> > if (f) {
> > rules[i] = >cr;
> > -
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Hi Kumar/Harry,
Find my comments inline.
//Eelco
On 17 Jun 2021, at 18:27, Kumar Amber wrote:
> This patch introduces the mfex function pointers which allows
> the user to switch between different miniflow extract implementations
> which are provided by the OVS based on optimized ISA CPU.
>
> The user can query for the available minflow extract variants available
> for that CPU by following commands:
>
> $ovs-appctl dpif-netdev/miniflow-parser-get
>
> Similarly an user can set the miniflow implementation by the following
> command :
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set name
>
> This allow for more performance and flexibility to the user to choose
> the miniflow implementation according to the needs.
>
> Signed-off-by: Kumar Amber
> Co-authored-by: Harry van Haaren
> Signed-off-by: Harry van Haaren
> ---
> lib/automake.mk | 2 +
> lib/dpif-netdev-avx512.c | 32 ++--
> lib/dpif-netdev-private-extract.c | 86
> lib/dpif-netdev-private-extract.h | 94 ++
> lib/dpif-netdev-private-thread.h | 4 +
> lib/dpif-netdev.c | 126 +-
> 6 files changed, 337 insertions(+), 7 deletions(-)
> create mode 100644 lib/dpif-netdev-private-extract.c
> create mode 100644 lib/dpif-netdev-private-extract.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 49f42c2a3..6657b9ae5 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> lib/dpif-netdev-private-dpcls.h \
> lib/dpif-netdev-private-dpif.c \
> lib/dpif-netdev-private-dpif.h \
> + lib/dpif-netdev-private-extract.c \
> + lib/dpif-netdev-private-extract.h \
> lib/dpif-netdev-private-flow.h \
> lib/dpif-netdev-private-hwol.h \
> lib/dpif-netdev-private-thread.h \
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index f9b199637..bb99b23ff 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -148,6 +148,15 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> * // do all processing (HWOL->MFEX->EMC->SMC)
> * }
> */
> +
> +/* Do a batch minfilow extract into keys. */
> +uint32_t mf_mask = 0;
> +if (pmd->miniflow_extract_opt) {
This will need some atomic get/use, or else we will crash here on change.
> +mf_mask = pmd->miniflow_extract_opt(packets, keys,
> +batch_size, in_port,
> +(void *) pmd);
Don’t think we should cast to void here, but I guess the callback should take
struct dp_netdev_pmd_thread?
> +}
> +/* Perform first packet interation */
> uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
> uint32_t iter = lookup_pkts_bitmask;
> while (iter) {
> @@ -159,6 +168,12 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> pkt_metadata_init(>md, in_port);
>
> struct dp_netdev_flow *f = NULL;
> +struct netdev_flow_key *key = [i];
> +
> +/* Check the minfiflow mask to see if the packet was correctly
> +* classifed by vector mfex else do a scalar miniflow extract
> +* for that packet. */
> +uint32_t mfex_hit = (mf_mask & (1 << i));
>
> /* Check for partial hardware offload mark. */
> uint32_t mark;
> @@ -166,7 +181,13 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> f = mark_to_flow_find(pmd, mark);
> if (f) {
> rules[i] = >cr;
> -pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> +/* If AVX512 MFEX already classified the packet, use it. */
> +if (mfex_hit) {
> +pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
> +} else {
> +pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> +}
> +
> pkt_meta[i].bytes = dp_packet_size(packet);
> phwol_hits++;
> hwol_emc_smc_hitmask |= (1 << i);
> @@ -174,11 +195,12 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> }
> }
>
> -/* Do miniflow extract into keys. */
> -struct netdev_flow_key *key = [i];
> -miniflow_extract(packet, >mf);
> +if (!mfex_hit) {
> +/* Do a scalar miniflow extract into keys */
> +miniflow_extract(packet, >mf);
> +}
>
> -/* Cache TCP and byte values for all packets. */
> +/* Cache TCP and byte values for all packets */
> pkt_meta[i].bytes = dp_packet_size(packet);
> pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
>
> diff --git a/lib/dpif-netdev-private-extract.c
> b/lib/dpif-netdev-private-extract.c
> new file mode 100644
> index 0..fcc56ef26
> --- /dev/null
> +++
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Hi Flavio,
Thanks a lot for the review my replies are inline.
> > +uint32_t mfex_hit = (mf_mask & (1 << i));
>
> This is actually a bool.
>
Fixed in upcoming v5.
> >
> > /* Check for partial hardware offload mark. */
> > uint32_t mark;
>
> I find 'ovs-appctl dpif-netdev/miniflow-parser-set disable'
> misleading because we are not disabling miniflow parser at all.
> It is just using scalar function instead.
>
Fixed in upcoming v5. Changed from Disable to scalar.
>
> > +};
> > +
> > +BUILD_ASSERT_DECL(MFEX_IMPLS_MAX_SIZE > ARRAY_SIZE(mfex_impls));
> > +
> > +int32_t
> > +dpif_miniflow_extract_opt_get(const char *name,
> > + struct dpif_miniflow_extract_impl
> > +**opt) {
> > +ovs_assert(opt);
> > +ovs_assert(name);
> > +
> > +uint32_t i;
> > +for (i = 0; i < ARRAY_SIZE(mfex_impls); i++) {
> > +if (strcmp(name, mfex_impls[i].name) == 0) {
> > +*opt = _impls[i];
> > +return 0;
> > +}
> > +}
> > +return -ENOTSUP;
> > +}
> > +
> > +void
> > +dpif_miniflow_extract_init(void)
> > +{
> > +/* Call probe on each impl, and cache the result. */
> > +uint32_t i;
> > +for (i = 0; i < ARRAY_SIZE(mfex_impls); i++) {
> > +int avail = 1;
>
> Can this be a bool instead?
Fixed in upcoming v5.
>
> > +if (mfex_impls[i].probe) {
> > +/* Return zero is success, non-zero means error. */
> > +avail = (mfex_impls[i].probe() == 0);
> > +}
> > +VLOG_INFO("Miniflow Extract implementation %s (available: %s)\n",
> > + mfex_impls[i].name, avail ? "True" : "False");
> > +mfex_impls[i].available = avail;
> > +}
> > +}
> > +
> > +int32_t
> > +dpif_miniflow_extract_info_get(struct dpif_miniflow_extract_impl
> > +**out_ptr) {
> > +if (out_ptr == NULL) {
> > +return -EINVAL;
> > +}
>
> missing space.
Fixed in upcoming v5.
>
> > +*out_ptr = mfex_impls;
> > +return ARRAY_SIZE(mfex_impls);
> > +}
> > diff --git a/lib/dpif-netdev-private-extract.h
> > +struct netdev_flow_key;
> > +
> > +/* Function pointer prototype to be implemented in the optimized
> > +miniflow
> > + * extract code.
> > + * returns the hitmask of the processed packets on success.
> > + * returns zero on failure.
> > + */
> > +typedef uint32_t (*miniflow_extract_func)(struct dp_packet_batch
> *batch,
> > + struct netdev_flow_key *keys,
> > + uint32_t keys_size,
> > + odp_port_t in_port,
> > + void *pmd_handle);
>
> The pmd_handle is void here, but some callers in future patches cast it to
> struct dp_netdev_pmd_thread. After the refactoring, that struct is declared
> in dpif-netdev-private-thread.h, so can we use that instead?
>
>
Fixed in upcoming v5. Changed structure than void.
> > +
> > +/* Probe function is used to detect if this CPU has the ISA required
> > + * to run the optimized miniflow implementation.
> > + * returns one on successful probe.
> > + * returns zero on failure.
> > + */
> > +typedef int32_t (*miniflow_extract_probe)(void);
> > +
> > +/* Structure representing the attributes of an optimized
> > +implementation. */ struct dpif_miniflow_extract_impl {
> > +/* When non-zero, this impl has passed the probe() checks. */
> > +uint8_t available;
>
> Can this be of type bool?
>
Fixed in upcoming v5.
> > +
> > +/* Probe function is used to detect if this CPU has the ISA required
> > + * to run the optimized miniflow implementation.
> > + */
> > +miniflow_extract_probe probe;
> > +
> > +/* Function to call to extract miniflows for a burst of packets. */
> > +miniflow_extract_func extract_func;
> > +
> > +/* Name of the optimized implementation. */
> > +char *name;
> > +};
> > +
> > +/* Retrieve the opt structure for the requested implementation by name.
> > + * Returns zero on success, and opt points to a valid struct, or
> > + * returns a negative failure status.
> > + * -ENOTSUP : invalid name requested
> > + */
> > +int32_t
> > +dpif_miniflow_extract_opt_get(const char *name,
> > + struct dpif_miniflow_extract_impl
> > +**opt);
> > +
> > +/* Initializes the available miniflow extract implementations by
> > +probing for
> > + * the CPU ISA requirements. As the runtime available CPU ISA does
> > +not change
> > + * and the required ISA of the implementation also does not change,
> > +it is safe
> > + * to cache the probe() results, and not call probe() at runtime.
> > + */
> > +void
> > +dpif_miniflow_extract_init(void);
> > +
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > f316835a4..567ebd952 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -46,6 +46,7 @@
> > #include "dpif.h"
> > #include "dpif-netdev-lookup.h"
> > #include
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Hi,
I am reviewing this patch ignoring the things that were
already pointed out in other reviews in the ML.
On Thu, Jun 17, 2021 at 09:57:43PM +0530, Kumar Amber wrote:
> This patch introduces the mfex function pointers which allows
> the user to switch between different miniflow extract implementations
> which are provided by the OVS based on optimized ISA CPU.
>
> The user can query for the available minflow extract variants available
> for that CPU by following commands:
>
> $ovs-appctl dpif-netdev/miniflow-parser-get
>
> Similarly an user can set the miniflow implementation by the following
> command :
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set name
>
> This allow for more performance and flexibility to the user to choose
> the miniflow implementation according to the needs.
>
> Signed-off-by: Kumar Amber
> Co-authored-by: Harry van Haaren
> Signed-off-by: Harry van Haaren
> ---
> lib/automake.mk | 2 +
> lib/dpif-netdev-avx512.c | 32 ++--
> lib/dpif-netdev-private-extract.c | 86
> lib/dpif-netdev-private-extract.h | 94 ++
> lib/dpif-netdev-private-thread.h | 4 +
> lib/dpif-netdev.c | 126 +-
> 6 files changed, 337 insertions(+), 7 deletions(-)
> create mode 100644 lib/dpif-netdev-private-extract.c
> create mode 100644 lib/dpif-netdev-private-extract.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 49f42c2a3..6657b9ae5 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> lib/dpif-netdev-private-dpcls.h \
> lib/dpif-netdev-private-dpif.c \
> lib/dpif-netdev-private-dpif.h \
> + lib/dpif-netdev-private-extract.c \
> + lib/dpif-netdev-private-extract.h \
> lib/dpif-netdev-private-flow.h \
> lib/dpif-netdev-private-hwol.h \
> lib/dpif-netdev-private-thread.h \
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index f9b199637..bb99b23ff 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -148,6 +148,15 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> * // do all processing (HWOL->MFEX->EMC->SMC)
> * }
> */
> +
> +/* Do a batch minfilow extract into keys. */
> +uint32_t mf_mask = 0;
> +if (pmd->miniflow_extract_opt) {
> +mf_mask = pmd->miniflow_extract_opt(packets, keys,
> +batch_size, in_port,
> +(void *) pmd);
> +}
> +/* Perform first packet interation */
> uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
> uint32_t iter = lookup_pkts_bitmask;
> while (iter) {
> @@ -159,6 +168,12 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> pkt_metadata_init(>md, in_port);
>
> struct dp_netdev_flow *f = NULL;
> +struct netdev_flow_key *key = [i];
> +
> +/* Check the minfiflow mask to see if the packet was correctly
> +* classifed by vector mfex else do a scalar miniflow extract
> +* for that packet. */
> +uint32_t mfex_hit = (mf_mask & (1 << i));
This is actually a bool.
>
> /* Check for partial hardware offload mark. */
> uint32_t mark;
> @@ -166,7 +181,13 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> f = mark_to_flow_find(pmd, mark);
> if (f) {
> rules[i] = >cr;
> -pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> +/* If AVX512 MFEX already classified the packet, use it. */
> +if (mfex_hit) {
> +pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
> +} else {
> +pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> +}
> +
> pkt_meta[i].bytes = dp_packet_size(packet);
> phwol_hits++;
> hwol_emc_smc_hitmask |= (1 << i);
> @@ -174,11 +195,12 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> }
> }
>
> -/* Do miniflow extract into keys. */
> -struct netdev_flow_key *key = [i];
> -miniflow_extract(packet, >mf);
> +if (!mfex_hit) {
> +/* Do a scalar miniflow extract into keys */
> +miniflow_extract(packet, >mf);
> +}
>
> -/* Cache TCP and byte values for all packets. */
> +/* Cache TCP and byte values for all packets */
> pkt_meta[i].bytes = dp_packet_size(packet);
> pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
>
> diff --git a/lib/dpif-netdev-private-extract.c
> b/lib/dpif-netdev-private-extract.c
> new file mode 100644
> index 0..fcc56ef26
> --- /dev/null
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -0,0 +1,86 @@
> +/*
> + * Copyright (c)
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Hi Ian ,
Thanks for the review , replies are inline.
> > This allow for more performance and flexibility to the user to choose
>
> Minor typo above, allow -> allows.
>
Included in v5
> > the miniflow implementation according to the needs.
> >
> > Signed-off-by: Kumar Amber
> > Co-authored-by: Harry van Haaren
> > Signed-off-by: Harry van Haaren
> > ---
> > lib/automake.mk | 2 +
> > lib/dpif-netdev-avx512.c | 32 ++--
> > lib/dpif-netdev-private-extract.c | 86
> > lib/dpif-netdev-private-extract.h | 94 ++
> > lib/dpif-netdev-private-thread.h | 4 +
> > lib/dpif-netdev.c | 126 +-
> > 6 files changed, 337 insertions(+), 7 deletions(-) create mode
> > 100644 lib/dpif-netdev-private-extract.c create mode 100644
> > lib/dpif-netdev-private-extract.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk index
> > 49f42c2a3..6657b9ae5 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> > lib/dpif-netdev-private-dpcls.h \ lib/dpif-netdev-private-dpif.c \
> > lib/dpif-netdev-private-dpif.h \
> > +lib/dpif-netdev-private-extract.c \
> > +lib/dpif-netdev-private-extract.h \
> > lib/dpif-netdev-private-flow.h \
> > lib/dpif-netdev-private-hwol.h \
> > lib/dpif-netdev-private-thread.h \
> > diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c index
> > f9b199637..bb99b23ff 100644
> > --- a/lib/dpif-netdev-avx512.c
> > +++ b/lib/dpif-netdev-avx512.c
> > @@ -148,6 +148,15 @@ dp_netdev_input_outer_avx512(struct
> > dp_netdev_pmd_thread *pmd,
> > * // do all processing (HWOL->MFEX->EMC->SMC)
> > * }
> > */
> > +
> > +/* Do a batch minfilow extract into keys. */
> > +uint32_t mf_mask = 0;
> > +if (pmd->miniflow_extract_opt) {
> > +mf_mask = pmd->miniflow_extract_opt(packets, keys,
> > +batch_size, in_port,
> > +(void *) pmd);
> > +}
> > +/* Perform first packet interation */
> Would add whitespace above to separate the comment form the conditional
> block. Also minor nit, missing period at end of comment.
>
Included in v5
> > uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
> > uint32_t iter = lookup_pkts_bitmask;
> > while (iter) {
> > @@ -159,6 +168,12 @@ dp_netdev_input_outer_avx512(struct
> > dp_netdev_pmd_thread *pmd,
> > pkt_metadata_init(>md, in_port);
> >
> > struct dp_netdev_flow *f = NULL;
> > +struct netdev_flow_key *key = [i];
> > +
> > +/* Check the minfiflow mask to see if the packet was correctly
> > +* classifed by vector mfex else do a scalar miniflow extract
> > +* for that packet. */
> Typo above for miniflow. Also alignment of * in comment seems out of
> place.
> Should be vertically aligned. Would also suggest finishing with */ on
> separate line after the comment.
>
Included in v5
> > +uint32_t mfex_hit = (mf_mask & (1 << i));
> >
> > /* Check for partial hardware offload mark. */
> > uint32_t mark;
> > @@ -166,7 +181,13 @@ dp_netdev_input_outer_avx512(struct
> > dp_netdev_pmd_thread *pmd,
> > f = mark_to_flow_find(pmd, mark);
> > if (f) {
> > rules[i] = >cr;
> > -pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> > +/* If AVX512 MFEX already classified the packet, use it. */
> > +if (mfex_hit) {
> > +pkt_meta[i].tcp_flags =
> > miniflow_get_tcp_flags(>mf);
> > +} else {
> > +pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> > +}
> > +
> > pkt_meta[i].bytes = dp_packet_size(packet);
> > phwol_hits++;
> > hwol_emc_smc_hitmask |= (1 << i); @@ -174,11 +195,12
> > @@ dp_netdev_input_outer_avx512(struct
> > dp_netdev_pmd_thread *pmd,
> > }
> > }
> >
> > -/* Do miniflow extract into keys. */
> > -struct netdev_flow_key *key = [i];
> > -miniflow_extract(packet, >mf);
> > +if (!mfex_hit) {
> > +/* Do a scalar miniflow extract into keys */
> Minor, missing period in comment.
>
Included in v5
> > +miniflow_extract(packet, >mf);
> > +}
> >
> > -/* Cache TCP and byte values for all packets. */
> > +/* Cache TCP and byte values for all packets */
> Period removed from comment, should be put back.
>
Included in v5
> > pkt_meta[i].bytes = dp_packet_size(packet);
> > pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
> >
> > diff --git a/lib/dpif-netdev-private-extract.c
> > b/lib/dpif-netdev-private-extract.c
> > new file mode 100644
> > index 0..fcc56ef26
> > --- /dev/null
> > +++ b/lib/dpif-netdev-private-extract.c
> >
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
> This patch introduces the mfex function pointers which allows
> the user to switch between different miniflow extract implementations
> which are provided by the OVS based on optimized ISA CPU.
Thanks for the patch Amber/harry. Comments inline below.
>
> The user can query for the available minflow extract variants available
> for that CPU by following commands:
>
> $ovs-appctl dpif-netdev/miniflow-parser-get
>
> Similarly an user can set the miniflow implementation by the following
> command :
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set name
>
> This allow for more performance and flexibility to the user to choose
Minor typo above, allow -> allows.
> the miniflow implementation according to the needs.
>
> Signed-off-by: Kumar Amber
> Co-authored-by: Harry van Haaren
> Signed-off-by: Harry van Haaren
> ---
> lib/automake.mk | 2 +
> lib/dpif-netdev-avx512.c | 32 ++--
> lib/dpif-netdev-private-extract.c | 86
> lib/dpif-netdev-private-extract.h | 94 ++
> lib/dpif-netdev-private-thread.h | 4 +
> lib/dpif-netdev.c | 126 +-
> 6 files changed, 337 insertions(+), 7 deletions(-)
> create mode 100644 lib/dpif-netdev-private-extract.c
> create mode 100644 lib/dpif-netdev-private-extract.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 49f42c2a3..6657b9ae5 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> lib/dpif-netdev-private-dpcls.h \
> lib/dpif-netdev-private-dpif.c \
> lib/dpif-netdev-private-dpif.h \
> + lib/dpif-netdev-private-extract.c \
> + lib/dpif-netdev-private-extract.h \
> lib/dpif-netdev-private-flow.h \
> lib/dpif-netdev-private-hwol.h \
> lib/dpif-netdev-private-thread.h \
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index f9b199637..bb99b23ff 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -148,6 +148,15 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> * // do all processing (HWOL->MFEX->EMC->SMC)
> * }
> */
> +
> +/* Do a batch minfilow extract into keys. */
> +uint32_t mf_mask = 0;
> +if (pmd->miniflow_extract_opt) {
> +mf_mask = pmd->miniflow_extract_opt(packets, keys,
> +batch_size, in_port,
> +(void *) pmd);
> +}
> +/* Perform first packet interation */
Would add whitespace above to separate the comment form the conditional block.
Also minor nit, missing period at end of comment.
> uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
> uint32_t iter = lookup_pkts_bitmask;
> while (iter) {
> @@ -159,6 +168,12 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> pkt_metadata_init(>md, in_port);
>
> struct dp_netdev_flow *f = NULL;
> +struct netdev_flow_key *key = [i];
> +
> +/* Check the minfiflow mask to see if the packet was correctly
> +* classifed by vector mfex else do a scalar miniflow extract
> +* for that packet. */
Typo above for miniflow. Also alignment of * in comment seems out of place.
Should be vertically aligned. Would also suggest finishing with */ on separate
line after the comment.
> +uint32_t mfex_hit = (mf_mask & (1 << i));
>
> /* Check for partial hardware offload mark. */
> uint32_t mark;
> @@ -166,7 +181,13 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> f = mark_to_flow_find(pmd, mark);
> if (f) {
> rules[i] = >cr;
> -pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> +/* If AVX512 MFEX already classified the packet, use it. */
> +if (mfex_hit) {
> +pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
> +} else {
> +pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
> +}
> +
> pkt_meta[i].bytes = dp_packet_size(packet);
> phwol_hits++;
> hwol_emc_smc_hitmask |= (1 << i);
> @@ -174,11 +195,12 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> }
> }
>
> -/* Do miniflow extract into keys. */
> -struct netdev_flow_key *key = [i];
> -miniflow_extract(packet, >mf);
> +if (!mfex_hit) {
> +/* Do a scalar miniflow extract into keys */
Minor, missing period in comment.
> +miniflow_extract(packet, >mf);
> +}
>
> -/* Cache TCP and byte values for all packets. */
> +/* Cache TCP and byte values for all packets */
Period removed from comment, should be put back.
> pkt_meta[i].bytes = dp_packet_size(packet);
>
Re: [ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Bleep bloop. Greetings Kumar Amber, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 dpif-netdev: Add command line and function pointer for miniflow extract When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [v4 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
This patch introduces the mfex function pointers which allows
the user to switch between different miniflow extract implementations
which are provided by the OVS based on optimized ISA CPU.
The user can query for the available minflow extract variants available
for that CPU by following commands:
$ovs-appctl dpif-netdev/miniflow-parser-get
Similarly an user can set the miniflow implementation by the following
command :
$ ovs-appctl dpif-netdev/miniflow-parser-set name
This allow for more performance and flexibility to the user to choose
the miniflow implementation according to the needs.
Signed-off-by: Kumar Amber
Co-authored-by: Harry van Haaren
Signed-off-by: Harry van Haaren
---
lib/automake.mk | 2 +
lib/dpif-netdev-avx512.c | 32 ++--
lib/dpif-netdev-private-extract.c | 86
lib/dpif-netdev-private-extract.h | 94 ++
lib/dpif-netdev-private-thread.h | 4 +
lib/dpif-netdev.c | 126 +-
6 files changed, 337 insertions(+), 7 deletions(-)
create mode 100644 lib/dpif-netdev-private-extract.c
create mode 100644 lib/dpif-netdev-private-extract.h
diff --git a/lib/automake.mk b/lib/automake.mk
index 49f42c2a3..6657b9ae5 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
lib/dpif-netdev-private-dpcls.h \
lib/dpif-netdev-private-dpif.c \
lib/dpif-netdev-private-dpif.h \
+ lib/dpif-netdev-private-extract.c \
+ lib/dpif-netdev-private-extract.h \
lib/dpif-netdev-private-flow.h \
lib/dpif-netdev-private-hwol.h \
lib/dpif-netdev-private-thread.h \
diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index f9b199637..bb99b23ff 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -148,6 +148,15 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
*pmd,
* // do all processing (HWOL->MFEX->EMC->SMC)
* }
*/
+
+/* Do a batch minfilow extract into keys. */
+uint32_t mf_mask = 0;
+if (pmd->miniflow_extract_opt) {
+mf_mask = pmd->miniflow_extract_opt(packets, keys,
+batch_size, in_port,
+(void *) pmd);
+}
+/* Perform first packet interation */
uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
uint32_t iter = lookup_pkts_bitmask;
while (iter) {
@@ -159,6 +168,12 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
*pmd,
pkt_metadata_init(>md, in_port);
struct dp_netdev_flow *f = NULL;
+struct netdev_flow_key *key = [i];
+
+/* Check the minfiflow mask to see if the packet was correctly
+* classifed by vector mfex else do a scalar miniflow extract
+* for that packet. */
+uint32_t mfex_hit = (mf_mask & (1 << i));
/* Check for partial hardware offload mark. */
uint32_t mark;
@@ -166,7 +181,13 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
*pmd,
f = mark_to_flow_find(pmd, mark);
if (f) {
rules[i] = >cr;
-pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
+/* If AVX512 MFEX already classified the packet, use it. */
+if (mfex_hit) {
+pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
+} else {
+pkt_meta[i].tcp_flags = parse_tcp_flags(packet);
+}
+
pkt_meta[i].bytes = dp_packet_size(packet);
phwol_hits++;
hwol_emc_smc_hitmask |= (1 << i);
@@ -174,11 +195,12 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
*pmd,
}
}
-/* Do miniflow extract into keys. */
-struct netdev_flow_key *key = [i];
-miniflow_extract(packet, >mf);
+if (!mfex_hit) {
+/* Do a scalar miniflow extract into keys */
+miniflow_extract(packet, >mf);
+}
-/* Cache TCP and byte values for all packets. */
+/* Cache TCP and byte values for all packets */
pkt_meta[i].bytes = dp_packet_size(packet);
pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
diff --git a/lib/dpif-netdev-private-extract.c
b/lib/dpif-netdev-private-extract.c
new file mode 100644
index 0..fcc56ef26
--- /dev/null
+++ b/lib/dpif-netdev-private-extract.c
@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) 2021 Intel.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR
