Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On 13 Jul 2021, at 5:37, Amber, Kumar wrote: > Hi Eelco, Flavio , > > Pls find my comments Inline. > ACK, replied to in v10 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Hi Eelco, Flavio ,
Pls find my comments Inline.
> > +/* For the first call, this will be choosen based on the
> > + * compile time flag and if nor flag is set it is set to
> > + * default scalar.
> > + */
> > +if (OVS_UNLIKELY(!default_mfex_func_set)) {
> > +VLOG_INFO("Default MFEX implementation is %s.\n",
> > + mfex_impls[mfex_idx].name);
> > +atomic_store_relaxed(mfex_func, (uintptr_t) mfex_impls
> > + [mfex_idx].extract_func);
> > +default_mfex_func_set = true;
>
> If this only needs to be done once, why not move it to
> dpif_miniflow_extract_init() as suggested during the v6 review (in a later
> patch)?
> This will remove this check every time dp_mfex_impl_get_default() gets called.
>
Yes it does make .
> > +}
> > +
> > +return default_mfex_func;
> > +}
> > +
> > +int
> > +dp_mfex_impl_set_default_by_name(const char *name) {
> > +miniflow_extract_func new_default;
> > +atomic_uintptr_t *mfex_func = (void *)_mfex_func;
> > +
> > +int err = dp_mfex_impl_get_by_name(name, _default);
> > +
> > +if (!err) {
> > +atomic_store_relaxed(mfex_func, (uintptr_t) new_default);
> > +}
> > +
> > +return err;
> > +
> > +}
> > +
> > +void
> > +dp_mfex_impl_get(struct ds *reply, struct dp_netdev_pmd_thread
> **pmd_list,
> > + size_t pmd_list_size) {
> > +/* Add all MFEX functions to reply string. */
> > +ds_put_cstr(reply, "Available MFEX implementations:\n");
> > +
> > +for (int i = 0; i < MFEX_IMPL_MAX; i++) {
> > +ds_put_format(reply, " %s (available: %s pmds: ",
> > + mfex_impls[i].name, mfex_impls[i].available ?
> > + "True" : "False");
>
> Flavio mentioned that True/False did not make sense to an end-user, not sure
> if
> he has the same feeling here?
> Maybe yes/no make more sense here? Flavio?
>
Changes to available same as previous comments.
> > +
> > +for (size_t j = 0; j < pmd_list_size; j++) {
> > +struct dp_netdev_pmd_thread *pmd = pmd_list[j];
> > +if (pmd->core_id == NON_PMD_CORE_ID) {
> > +continue;
> > +}
> > +
> > +if (pmd->miniflow_extract_opt == mfex_impls[i].extract_func) {
> > +ds_put_format(reply, "%u,", pmd->core_id);
> > +}
> > +}
> > +
> > +ds_chomp(reply, ',');
> > +
> > +if (ds_last(reply) == ' ') {
> > +ds_put_cstr(reply, "none");
> > +}
> > +
> > +ds_put_cstr(reply, ")\n");
> > +}
> > +
> > +}
> > +
> > +/* This function checks all available MFEX implementations, and
> > +selects and
> > + * returns the function pointer to the one requested by "name". If
> > +nothing
> > + * is found it reutrns error.
>
> reutrns -> returns
>
Fixed.
> > + */
> > +int
> > +dp_mfex_impl_get_by_name(const char *name, miniflow_extract_func
> > +*out_func) {
> > +if ((name == NULL) || (out_func == NULL)) {
> > +return -EINVAL;
> > +}
> > +
> > +for (int i = 0; i < MFEX_IMPL_MAX; i++) {
> > +if (strcmp(mfex_impls[i].name, name) == 0) {
> > +/* Check available is set before exec. */
> > +if (!mfex_impls[i].available) {
> > +*out_func = NULL;
> > +return -ENODEV;
> > +}
> > +
> > +*out_func = mfex_impls[i].extract_func;
> > +return 0;
> > +}
> > +}
> > +
> > +return -ENOENT;
> > +}
> > diff --git a/lib/dpif-netdev-private-extract.h
> > b/lib/dpif-netdev-private-extract.h
> > new file mode 100644
> > index 0..ddf2e2845
> > --- /dev/null
> > +++ b/lib/dpif-netdev-private-extract.h
> > @@ -0,0 +1,111 @@
> > +/*
> > + * 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 CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions
> > +and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef MFEX_AVX512_EXTRACT
> > +#define MFEX_AVX512_EXTRACT 1
> > +
> > +#include
> > +
> > +/* Forward declarations. */
> > +struct dp_packet;
> > +struct miniflow;
> > +struct dp_netdev_pmd_thread;
> > +struct dp_packet_batch;
> > +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
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Aaron Conole writes: > Ilya Maximets writes: > >> On 7/12/21 5:10 PM, David Marchand wrote: >>> On Mon, Jul 12, 2021 at 4:43 PM Ilya Maximets wrote: >> ovsrobot has issues with reporting the status right now, but this >> patch fails the build in GHA: >> https://github.com/ovsrobot/ovs/actions/runs/1021787643 > > Thanks for linking on results. > > I've spot-checked a bunch of the failing builds, and found 2 fixable code > issues. > A few of the CI run's I can't find/explain the error, but I don't know of > a good > way to "jump to the error" line, am I missing a trick, or is scrolling > the whole > compiler output and checking errors the best method? typing 'error:' in the 'Search logs' field, usually gets you to the actual error faster, but, unfortunately, scrolling is the most reliable option. >>> >>> GHA ui jumps at the last line of a failing step, but the problem is >>> that, in OVS, we dump all logs which adds a lot of noise. >>> >>> We could stop dumping them, since those logs are attached to the job >>> as an archive. >>> Like what is done in DPDK. >>> http://git.dpdk.org/dpdk/tree/.ci/linux-build.sh#n3 >>> >>> WDYT? >> >> Yes, that is good thing to do. We didn't do that because of >> Travis CI, where we have no artifacts collected. > > +1 - we should bend over backwards to make things easier on Travis CI to > the detriment of other platforms. And by this, I mean the opposite - we should *NOT* bend over backwards to make things easier on Travis CI. >> But yes, checking for [ -n "$GITHUB_WORKFLOW" ] is a solution. >> >> Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
Ilya Maximets writes: > On 7/12/21 5:10 PM, David Marchand wrote: >> On Mon, Jul 12, 2021 at 4:43 PM Ilya Maximets wrote: > ovsrobot has issues with reporting the status right now, but this > patch fails the build in GHA: > https://github.com/ovsrobot/ovs/actions/runs/1021787643 Thanks for linking on results. I've spot-checked a bunch of the failing builds, and found 2 fixable code issues. A few of the CI run's I can't find/explain the error, but I don't know of a good way to "jump to the error" line, am I missing a trick, or is scrolling the whole compiler output and checking errors the best method? >>> >>> typing 'error:' in the 'Search logs' field, usually gets you >>> to the actual error faster, but, unfortunately, scrolling is >>> the most reliable option. >> >> GHA ui jumps at the last line of a failing step, but the problem is >> that, in OVS, we dump all logs which adds a lot of noise. >> >> We could stop dumping them, since those logs are attached to the job >> as an archive. >> Like what is done in DPDK. >> http://git.dpdk.org/dpdk/tree/.ci/linux-build.sh#n3 >> >> WDYT? > > Yes, that is good thing to do. We didn't do that because of > Travis CI, where we have no artifacts collected. +1 - we should bend over backwards to make things easier on Travis CI to the detriment of other platforms. > But yes, checking for [ -n "$GITHUB_WORKFLOW" ] is a solution. > > Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On 7/12/21 4:57 PM, Van Haaren, Harry wrote: >> -Original Message- >> From: Ilya Maximets >> Sent: Monday, July 12, 2021 3:43 PM >> To: Van Haaren, Harry ; Ilya Maximets >> ; Amber, Kumar ; ovs- >> d...@openvswitch.org >> Cc: f...@sysclose.org; echau...@redhat.com; Ferriter, Cian >> ; Stokes, Ian >> Subject: Re: [v9 01/12] dpif-netdev: Add command line and function pointer >> for >> miniflow extract >> >> On 7/12/21 4:02 PM, Van Haaren, Harry wrote: -Original Message- From: Ilya Maximets Sent: Monday, July 12, 2021 2:25 PM To: Amber, Kumar ; ovs-dev@openvswitch.org Cc: f...@sysclose.org; echau...@redhat.com; i.maxim...@ovn.org; Van Haaren, Harry ; Ferriter, Cian ; Stokes, Ian Subject: Re: [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract On 7/12/21 7:51 AM, kumar Amber wrote: > From: Kumar Amber > > 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 allows 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 > > --- > v9: > - fix review comments from Flavio > v7: > - fix review comments(Eelco, Flavio) > v5: > - fix review comments(Ian, Flavio, Eelco) > - add enum to hold mfex indexes > - add new get and set implemenatations > - add Atomic set and get > --- ovsrobot has issues with reporting the status right now, but this patch fails the build in GHA: https://github.com/ovsrobot/ovs/actions/runs/1021787643 >>> >>> Thanks for linking on results. >>> >>> I've spot-checked a bunch of the failing builds, and found 2 fixable code >>> issues. >>> A few of the CI run's I can't find/explain the error, but I don't know of a >>> good >>> way to "jump to the error" line, am I missing a trick, or is scrolling the >>> whole >>> compiler output and checking errors the best method? >> >> typing 'error:' in the 'Search logs' field, usually gets you >> to the actual error faster, but, unfortunately, scrolling is >> the most reliable option. > > Okay, thanks. > > >>> ISSUES: >>> #1 : OVS Requires Mutex issue (Linux clang test dpdk build) >>> 1291../../lib/dpif-netdev-private-extract.h:87:53: error: use of undeclared >> identifier 'dp_netdev_mutex'; did you mean 'dp_netdev_input'? >>> 1292 size_t pmd_list_size) OVS_REQUIRES(dp_netdev_mutex); >>> >>> #2 : Unused Argument (As from mailing list review comment too, linux gcc >>> dpdk -- >> enable-shared) >>> 2353lib/dpif-netdev.c:1079:63: error: unused parameter ‘argc’ >>> [-Werror=unused- >> parameter] >>> 2354 dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc, >>> >>> #3 : Distcheck directory not valid? (linux gcc test 3.16 build. I cannot >>> explain this?) >>> make: *** [distcheck] Error 1 >>> 4490Makefile:5298: recipe for target 'distcheck' failed >>> 4491+ cat '*/_build/sub/tests/testsuite.log' >>> 4492cat: '*/_build/sub/tests/testsuite.log': No such file or directory >>> 4493Error: Process completed with exit code 1.> >>> SOLUTIONS: >>> #1, likely to forward-decl the "dp_netdev_mutex" to make it available >>> in the extract header file, and remove the "static" keyword so its no >>> longer limited >>> to the dpif-netdev.c compilation unit. >>> >>> #2 is a simple OVS_UNUSED as Eelco suggested during review. >>> >>> #3, I'm not sure where the DistCheck issue arise from, it seems to be >>> missing >> directories >>> during the test run? Input appreciated, as pushing & hoping tends to be a >>> tiresome >>> and long process. >> >> This is just a result of the previous build failure. Build >> never reached the testsuite phase, so there are no testsuite >> logs there. You should not see this problem once build is >> fixed. > > Aha, good to know. Then a respin with the fixes for the above issues is > our next step, will arrive on the mailing list soon. If you have a github account it might be good to push patches one-by-one there to be sure that everything is fine before sending to the mail list to avoid re-spins due to build issues. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On 7/12/21 5:10 PM, David Marchand wrote: > On Mon, Jul 12, 2021 at 4:43 PM Ilya Maximets wrote: ovsrobot has issues with reporting the status right now, but this patch fails the build in GHA: https://github.com/ovsrobot/ovs/actions/runs/1021787643 >>> >>> Thanks for linking on results. >>> >>> I've spot-checked a bunch of the failing builds, and found 2 fixable code >>> issues. >>> A few of the CI run's I can't find/explain the error, but I don't know of a >>> good >>> way to "jump to the error" line, am I missing a trick, or is scrolling the >>> whole >>> compiler output and checking errors the best method? >> >> typing 'error:' in the 'Search logs' field, usually gets you >> to the actual error faster, but, unfortunately, scrolling is >> the most reliable option. > > GHA ui jumps at the last line of a failing step, but the problem is > that, in OVS, we dump all logs which adds a lot of noise. > > We could stop dumping them, since those logs are attached to the job > as an archive. > Like what is done in DPDK. > http://git.dpdk.org/dpdk/tree/.ci/linux-build.sh#n3 > > WDYT? Yes, that is good thing to do. We didn't do that because of Travis CI, where we have no artifacts collected. But yes, checking for [ -n "$GITHUB_WORKFLOW" ] is a solution. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On Mon, Jul 12, 2021 at 4:43 PM Ilya Maximets wrote: > >> ovsrobot has issues with reporting the status right now, but this > >> patch fails the build in GHA: > >> https://github.com/ovsrobot/ovs/actions/runs/1021787643 > > > > Thanks for linking on results. > > > > I've spot-checked a bunch of the failing builds, and found 2 fixable code > > issues. > > A few of the CI run's I can't find/explain the error, but I don't know of a > > good > > way to "jump to the error" line, am I missing a trick, or is scrolling the > > whole > > compiler output and checking errors the best method? > > typing 'error:' in the 'Search logs' field, usually gets you > to the actual error faster, but, unfortunately, scrolling is > the most reliable option. GHA ui jumps at the last line of a failing step, but the problem is that, in OVS, we dump all logs which adds a lot of noise. We could stop dumping them, since those logs are attached to the job as an archive. Like what is done in DPDK. http://git.dpdk.org/dpdk/tree/.ci/linux-build.sh#n3 WDYT? -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
> -Original Message- > From: Ilya Maximets > Sent: Monday, July 12, 2021 3:43 PM > To: Van Haaren, Harry ; Ilya Maximets > ; Amber, Kumar ; ovs- > d...@openvswitch.org > Cc: f...@sysclose.org; echau...@redhat.com; Ferriter, Cian > ; Stokes, Ian > Subject: Re: [v9 01/12] dpif-netdev: Add command line and function pointer for > miniflow extract > > On 7/12/21 4:02 PM, Van Haaren, Harry wrote: > >> -Original Message- > >> From: Ilya Maximets > >> Sent: Monday, July 12, 2021 2:25 PM > >> To: Amber, Kumar ; ovs-dev@openvswitch.org > >> Cc: f...@sysclose.org; echau...@redhat.com; i.maxim...@ovn.org; Van Haaren, > >> Harry ; Ferriter, Cian > >> ; > >> Stokes, Ian > >> Subject: Re: [v9 01/12] dpif-netdev: Add command line and function pointer > >> for > >> miniflow extract > >> > >> On 7/12/21 7:51 AM, kumar Amber wrote: > >>> From: Kumar Amber > >>> > >>> 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 allows 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 > >>> > >>> --- > >>> v9: > >>> - fix review comments from Flavio > >>> v7: > >>> - fix review comments(Eelco, Flavio) > >>> v5: > >>> - fix review comments(Ian, Flavio, Eelco) > >>> - add enum to hold mfex indexes > >>> - add new get and set implemenatations > >>> - add Atomic set and get > >>> --- > >> > >> ovsrobot has issues with reporting the status right now, but this > >> patch fails the build in GHA: > >> https://github.com/ovsrobot/ovs/actions/runs/1021787643 > > > > Thanks for linking on results. > > > > I've spot-checked a bunch of the failing builds, and found 2 fixable code > > issues. > > A few of the CI run's I can't find/explain the error, but I don't know of a > > good > > way to "jump to the error" line, am I missing a trick, or is scrolling the > > whole > > compiler output and checking errors the best method? > > typing 'error:' in the 'Search logs' field, usually gets you > to the actual error faster, but, unfortunately, scrolling is > the most reliable option. Okay, thanks. > > ISSUES: > > #1 : OVS Requires Mutex issue (Linux clang test dpdk build) > > 1291../../lib/dpif-netdev-private-extract.h:87:53: error: use of undeclared > identifier 'dp_netdev_mutex'; did you mean 'dp_netdev_input'? > > 1292 size_t pmd_list_size) OVS_REQUIRES(dp_netdev_mutex); > > > > #2 : Unused Argument (As from mailing list review comment too, linux gcc > > dpdk -- > enable-shared) > > 2353lib/dpif-netdev.c:1079:63: error: unused parameter ‘argc’ > > [-Werror=unused- > parameter] > > 2354 dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc, > > > > #3 : Distcheck directory not valid? (linux gcc test 3.16 build. I cannot > > explain this?) > > make: *** [distcheck] Error 1 > > 4490Makefile:5298: recipe for target 'distcheck' failed > > 4491+ cat '*/_build/sub/tests/testsuite.log' > > 4492cat: '*/_build/sub/tests/testsuite.log': No such file or directory > > 4493Error: Process completed with exit code 1.> > > SOLUTIONS: > > #1, likely to forward-decl the "dp_netdev_mutex" to make it available > > in the extract header file, and remove the "static" keyword so its no > > longer limited > > to the dpif-netdev.c compilation unit. > > > > #2 is a simple OVS_UNUSED as Eelco suggested during review. > > > > #3, I'm not sure where the DistCheck issue arise from, it seems to be > > missing > directories > > during the test run? Input appreciated, as pushing & hoping tends to be a > > tiresome > > and long process. > > This is just a result of the previous build failure. Build > never reached the testsuite phase, so there are no testsuite > logs there. You should not see this problem once build is > fixed. Aha, good to know. Then a respin with the fixes for the above issues is our next step, will arrive on the mailing list soon. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On 7/12/21 4:02 PM, Van Haaren, Harry wrote: >> -Original Message- >> From: Ilya Maximets >> Sent: Monday, July 12, 2021 2:25 PM >> To: Amber, Kumar ; ovs-dev@openvswitch.org >> Cc: f...@sysclose.org; echau...@redhat.com; i.maxim...@ovn.org; Van Haaren, >> Harry ; Ferriter, Cian ; >> Stokes, Ian >> Subject: Re: [v9 01/12] dpif-netdev: Add command line and function pointer >> for >> miniflow extract >> >> On 7/12/21 7:51 AM, kumar Amber wrote: >>> From: Kumar Amber >>> >>> 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 allows 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 >>> >>> --- >>> v9: >>> - fix review comments from Flavio >>> v7: >>> - fix review comments(Eelco, Flavio) >>> v5: >>> - fix review comments(Ian, Flavio, Eelco) >>> - add enum to hold mfex indexes >>> - add new get and set implemenatations >>> - add Atomic set and get >>> --- >> >> ovsrobot has issues with reporting the status right now, but this >> patch fails the build in GHA: >> https://github.com/ovsrobot/ovs/actions/runs/1021787643 > > Thanks for linking on results. > > I've spot-checked a bunch of the failing builds, and found 2 fixable code > issues. > A few of the CI run's I can't find/explain the error, but I don't know of a > good > way to "jump to the error" line, am I missing a trick, or is scrolling the > whole > compiler output and checking errors the best method? typing 'error:' in the 'Search logs' field, usually gets you to the actual error faster, but, unfortunately, scrolling is the most reliable option. > > ISSUES: > #1 : OVS Requires Mutex issue (Linux clang test dpdk build) > 1291../../lib/dpif-netdev-private-extract.h:87:53: error: use of undeclared > identifier 'dp_netdev_mutex'; did you mean 'dp_netdev_input'? > 1292 size_t pmd_list_size) OVS_REQUIRES(dp_netdev_mutex); > > #2 : Unused Argument (As from mailing list review comment too, linux gcc dpdk > --enable-shared) > 2353lib/dpif-netdev.c:1079:63: error: unused parameter ‘argc’ > [-Werror=unused-parameter] > 2354 dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc, > > #3 : Distcheck directory not valid? (linux gcc test 3.16 build. I cannot > explain this?) > make: *** [distcheck] Error 1 > 4490Makefile:5298: recipe for target 'distcheck' failed > 4491+ cat '*/_build/sub/tests/testsuite.log' > 4492cat: '*/_build/sub/tests/testsuite.log': No such file or directory > 4493Error: Process completed with exit code 1.> > SOLUTIONS: > #1, likely to forward-decl the "dp_netdev_mutex" to make it available > in the extract header file, and remove the "static" keyword so its no longer > limited > to the dpif-netdev.c compilation unit. > > #2 is a simple OVS_UNUSED as Eelco suggested during review. > > #3, I'm not sure where the DistCheck issue arise from, it seems to be missing > directories > during the test run? Input appreciated, as pushing & hoping tends to be a > tiresome > and long process. This is just a result of the previous build failure. Build never reached the testsuite phase, so there are no testsuite logs there. You should not see this problem once build is fixed. > > >> Best regards, Ilya Maximets. > > Regards, -Harry > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
> -Original Message- > From: Ilya Maximets > Sent: Monday, July 12, 2021 2:25 PM > To: Amber, Kumar ; ovs-dev@openvswitch.org > Cc: f...@sysclose.org; echau...@redhat.com; i.maxim...@ovn.org; Van Haaren, > Harry ; Ferriter, Cian ; > Stokes, Ian > Subject: Re: [v9 01/12] dpif-netdev: Add command line and function pointer for > miniflow extract > > On 7/12/21 7:51 AM, kumar Amber wrote: > > From: Kumar Amber > > > > 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 allows 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 > > > > --- > > v9: > > - fix review comments from Flavio > > v7: > > - fix review comments(Eelco, Flavio) > > v5: > > - fix review comments(Ian, Flavio, Eelco) > > - add enum to hold mfex indexes > > - add new get and set implemenatations > > - add Atomic set and get > > --- > > ovsrobot has issues with reporting the status right now, but this > patch fails the build in GHA: > https://github.com/ovsrobot/ovs/actions/runs/1021787643 Thanks for linking on results. I've spot-checked a bunch of the failing builds, and found 2 fixable code issues. A few of the CI run's I can't find/explain the error, but I don't know of a good way to "jump to the error" line, am I missing a trick, or is scrolling the whole compiler output and checking errors the best method? ISSUES: #1 : OVS Requires Mutex issue (Linux clang test dpdk build) 1291../../lib/dpif-netdev-private-extract.h:87:53: error: use of undeclared identifier 'dp_netdev_mutex'; did you mean 'dp_netdev_input'? 1292 size_t pmd_list_size) OVS_REQUIRES(dp_netdev_mutex); #2 : Unused Argument (As from mailing list review comment too, linux gcc dpdk --enable-shared) 2353lib/dpif-netdev.c:1079:63: error: unused parameter ‘argc’ [-Werror=unused-parameter] 2354 dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc, #3 : Distcheck directory not valid? (linux gcc test 3.16 build. I cannot explain this?) make: *** [distcheck] Error 1 4490Makefile:5298: recipe for target 'distcheck' failed 4491+ cat '*/_build/sub/tests/testsuite.log' 4492cat: '*/_build/sub/tests/testsuite.log': No such file or directory 4493Error: Process completed with exit code 1. SOLUTIONS: #1, likely to forward-decl the "dp_netdev_mutex" to make it available in the extract header file, and remove the "static" keyword so its no longer limited to the dpif-netdev.c compilation unit. #2 is a simple OVS_UNUSED as Eelco suggested during review. #3, I'm not sure where the DistCheck issue arise from, it seems to be missing directories during the test run? Input appreciated, as pushing & hoping tends to be a tiresome and long process. > Best regards, Ilya Maximets. Regards, -Harry ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On 7/12/21 7:51 AM, kumar Amber wrote: > From: Kumar Amber > > 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 allows 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 > > --- > v9: > - fix review comments from Flavio > v7: > - fix review comments(Eelco, Flavio) > v5: > - fix review comments(Ian, Flavio, Eelco) > - add enum to hold mfex indexes > - add new get and set implemenatations > - add Atomic set and get > --- ovsrobot has issues with reporting the status right now, but this patch fails the build in GHA: https://github.com/ovsrobot/ovs/actions/runs/1021787643 Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
On Mon, Jul 12, 2021 at 02:22:46PM +0200, Eelco Chaudron wrote:
> See some comments below…
>
> For this patch series, I’m only looking at the diff from v6..v9, not a full
> review.
> I will do basic compilation and some tests at the end.
>
> Cheers,
>
> Eelco
>
>
> On 12 Jul 2021, at 7:51, kumar Amber wrote:
>
> > From: Kumar Amber
> >
> > 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 allows 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
> >
> > ---
> > v9:
> > - fix review comments from Flavio
> > v7:
> > - fix review comments(Eelco, Flavio)
> > v5:
> > - fix review comments(Ian, Flavio, Eelco)
> > - add enum to hold mfex indexes
> > - add new get and set implemenatations
> > - add Atomic set and get
> > ---
> > ---
> > NEWS | 1 +
> > lib/automake.mk | 2 +
> > lib/dpif-netdev-avx512.c | 31 +-
> > lib/dpif-netdev-private-extract.c | 162 ++
> > lib/dpif-netdev-private-extract.h | 111
> > lib/dpif-netdev-private-thread.h | 8 ++
> > lib/dpif-netdev.c | 105 +++
> > 7 files changed, 416 insertions(+), 4 deletions(-)
> > create mode 100644 lib/dpif-netdev-private-extract.c
> > create mode 100644 lib/dpif-netdev-private-extract.h
> >
> > diff --git a/NEWS b/NEWS
> > index 6cdccc715..b0f08e96d 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -32,6 +32,7 @@ Post-v2.15.0
> > * Enable the AVX512 DPCLS implementation to use VPOPCNT instruction
> > if the
> > CPU supports it. This enhances performance by using the native
> > vpopcount
> > instructions, instead of the emulated version of vpopcount.
> > + * Add command line option to switch between MFEX function pointers.
> > - ovs-ctl:
> > * New option '--no-record-hostname' to disable hostname configuration
> > in ovsdb on startup.
> > diff --git a/lib/automake.mk b/lib/automake.mk
> > index 3c9523c1a..53b8abc0f 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-thread.h \
> > lib/dpif-netdev-private.h \
> > diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> > index 6f9aa8284..7772b7abf 100644
> > --- a/lib/dpif-netdev-avx512.c
> > +++ b/lib/dpif-netdev-avx512.c
> > @@ -149,6 +149,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;
> > +miniflow_extract_func mfex_func;
> > +atomic_read_relaxed(>miniflow_extract_opt, _func);
> > +if (mfex_func) {
> > +mf_mask = mfex_func(packets, keys, batch_size, in_port, pmd);
> > +}
> > +
> > uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
> > uint32_t iter = lookup_pkts_bitmask;
> > while (iter) {
> > @@ -167,6 +176,13 @@ 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.
> > + */
> > +bool mfex_hit = !!(mf_mask & (1 << i));
> >
> > /* Check for a partial hardware offload match. */
> > if (hwol_enabled) {
> > @@ -177,7 +193,13 @@ dp_netdev_input_outer_avx512(struct
> > dp_netdev_pmd_thread *pmd,
> > }
> > 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 {
> > +
Re: [ovs-dev] [v9 01/12] dpif-netdev: Add command line and function pointer for miniflow extract
See some comments below…
For this patch series, I’m only looking at the diff from v6..v9, not a full
review.
I will do basic compilation and some tests at the end.
Cheers,
Eelco
On 12 Jul 2021, at 7:51, kumar Amber wrote:
> From: Kumar Amber
>
> 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 allows 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
>
> ---
> v9:
> - fix review comments from Flavio
> v7:
> - fix review comments(Eelco, Flavio)
> v5:
> - fix review comments(Ian, Flavio, Eelco)
> - add enum to hold mfex indexes
> - add new get and set implemenatations
> - add Atomic set and get
> ---
> ---
> NEWS | 1 +
> lib/automake.mk | 2 +
> lib/dpif-netdev-avx512.c | 31 +-
> lib/dpif-netdev-private-extract.c | 162 ++
> lib/dpif-netdev-private-extract.h | 111
> lib/dpif-netdev-private-thread.h | 8 ++
> lib/dpif-netdev.c | 105 +++
> 7 files changed, 416 insertions(+), 4 deletions(-)
> create mode 100644 lib/dpif-netdev-private-extract.c
> create mode 100644 lib/dpif-netdev-private-extract.h
>
> diff --git a/NEWS b/NEWS
> index 6cdccc715..b0f08e96d 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -32,6 +32,7 @@ Post-v2.15.0
> * Enable the AVX512 DPCLS implementation to use VPOPCNT instruction if
> the
> CPU supports it. This enhances performance by using the native
> vpopcount
> instructions, instead of the emulated version of vpopcount.
> + * Add command line option to switch between MFEX function pointers.
> - ovs-ctl:
> * New option '--no-record-hostname' to disable hostname configuration
> in ovsdb on startup.
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 3c9523c1a..53b8abc0f 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-thread.h \
> lib/dpif-netdev-private.h \
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index 6f9aa8284..7772b7abf 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -149,6 +149,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;
> +miniflow_extract_func mfex_func;
> +atomic_read_relaxed(>miniflow_extract_opt, _func);
> +if (mfex_func) {
> +mf_mask = mfex_func(packets, keys, batch_size, in_port, pmd);
> +}
> +
> uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1;
> uint32_t iter = lookup_pkts_bitmask;
> while (iter) {
> @@ -167,6 +176,13 @@ 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.
> + */
> +bool mfex_hit = !!(mf_mask & (1 << i));
>
> /* Check for a partial hardware offload match. */
> if (hwol_enabled) {
> @@ -177,7 +193,13 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
> }
> 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);
> @@ -185,9 +207,10 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
Re: [ovs-dev] [v9 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. build: /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/dpif-netdev-lookup.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpif-netdev-lookup.lo lib/dpif-netdev-lookup.c &&\ mv -f $depbase.Tpo $depbase.Plo libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/dpif-netdev-lookup.lo -MD -MP -MF lib/.deps/dpif-netdev-lookup.Tpo -c lib/dpif-netdev-lookup.c -o lib/dpif-netdev-lookup.o depbase=`echo lib/dpif-netdev-lookup-autovalidator.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\ /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/dpif-netdev-lookup-autovalidator.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpif-netdev-lookup-autovalidator.lo lib/dpif-netdev-lookup-autovalidator.c &&\ mv -f $depbase.Tpo $depbase.Plo libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/dpif-netdev-lookup-autovalidator.lo -MD -MP -MF lib/.deps/dpif-netdev-lookup-autovalidator.Tpo -c lib/dpif-netdev-lookup-autovalidator.c -o lib/dpif-netdev-lookup-autovalidator.o depbase=`echo lib/dpif-netdev-lookup-generic.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\ /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/dpif-netdev-lookup-generic.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpif-netdev-lookup-generic.lo lib/dpif-netdev-lookup-generic.c &&\ mv -f $depbase.Tpo $depbase.Plo libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/dpif-netdev-lookup-generic.lo -MD -MP -MF lib/.deps/dpif-netdev-lookup-generic.Tpo -c lib/dpif-netdev-lookup-generic.c -o lib/dpif-netdev-lookup-generic.o depbase=`echo lib/dpif-netdev.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\ /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/dpif-netdev.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpif-netdev.lo lib/dpif-netdev.c &&\ mv -f $depbase.Tpo $depbase.Plo libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast
