[ovs-dev,1/2] Avoid packet drop on LACP bond after link up
diff mbox series

Message ID 1551285539-31225-1-git-send-email-nitin.katiyar@ericsson.com
State Superseded
Headers show
Series
  • [ovs-dev,1/2] Avoid packet drop on LACP bond after link up
Related show

Commit Message

Nitin Katiyar Feb. 27, 2019, 8:37 a.m. UTC
Problem:

Comments

0-day Robot Feb. 27, 2019, 8:58 a.m. UTC | #1
Bleep bloop.  Greetings Nitin Katiyar, 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 -MT lib/jhash.lo -MD -MP -MF $depbase.Tpo -c -o lib/jhash.lo lib/jhash.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 -MT lib/jhash.lo -MD -MP -MF lib/.deps/jhash.Tpo -c lib/jhash.c -o lib/jhash.o
depbase=`echo lib/json.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 -MT lib/json.lo -MD -MP -MF $depbase.Tpo -c -o lib/json.lo lib/json.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 -MT lib/json.lo -MD -MP -MF lib/.deps/json.Tpo -c lib/json.c -o lib/json.o
depbase=`echo lib/jsonrpc.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 -MT lib/jsonrpc.lo -MD -MP -MF $depbase.Tpo -c -o lib/jsonrpc.lo lib/jsonrpc.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 -MT lib/jsonrpc.lo -MD -MP -MF lib/.deps/jsonrpc.Tpo -c lib/jsonrpc.c -o lib/jsonrpc.o
depbase=`echo lib/lacp.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 -MT lib/lacp.lo -MD -MP -MF $depbase.Tpo -c -o lib/lacp.lo lib/lacp.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 -MT lib/lacp.lo -MD -MP -MF lib/.deps/lacp.Tpo -c lib/lacp.c -o lib/lacp.o
lib/lacp.c: In function 'lacp_process_packet':
lib/lacp.c:329:52: error: unused parameter 'bond' [-Werror=unused-parameter]
 lacp_process_packet(struct lacp *lacp, const void *bond, const void *slave_,
                                                    ^
cc1: all warnings being treated as errors
make[2]: *** [lib/lacp.lo] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Aaron Conole Feb. 27, 2019, 2:47 p.m. UTC | #2
Nitin Katiyar <nitin.katiyar@ericsson.com> writes:

> Hi,
> I got this mail reporting build failure. It complains for bond
> variable not being used. I had sent patch earlier after correcting
> this. So, I am not sure which patch version it is complaining for. In
> my latest patch I see that it is being used. Am I missing something?

Looking at the message ID to which you're replying:

  Robot mail: 1551257882-9544-1-git-send-email-robot@bytheb.org

Looking at that message id, it is a reply to:

  Nitin Katiyar: 1551285539-31225-1-git-send-email-nitin.katiyar@ericsson.com

Looking at that message, I see that it includes:

@@ -324,8 +325,8 @@ lacp_is_active(const struct lacp *lacp) OVS_EXCLUDED(mutex)
 /* Processes 'packet' which was received on 'slave_'.  This function should be
  * called on all packets received on 'slave_' with Ethernet Type ETH_TYPE_LACP.
  */
-void
-lacp_process_packet(struct lacp *lacp, const void *slave_,
+bool
+lacp_process_packet(struct lacp *lacp, const void *bond, const void *slave_,
                     const struct dp_packet *packet)
     OVS_EXCLUDED(mutex)
 {


Which indeed looks like the bond argument is added, but never referenced.

> On other hand, it will be good if report captures the patch or version of the patch.

I agree, I'll fix that up.

On the other hand, the email in question is at url:

https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356740.html

It does not contain a version string.  So it seems this is a valid
report.

> Regards,
> Nitin
>
> -----Original Message-----
> From: 0-day Robot [mailto:robot@bytheb.org] 
> Sent: Wednesday, February 27, 2019 2:28 PM
> To: Nitin Katiyar <nitin.katiyar@ericsson.com>
> Cc: dev@openvswitch.org
> Subject: Re: Avoid packet drop on LACP bond after link up
>
> Bleep bloop.  Greetings Nitin Katiyar, 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 -MT lib/jhash.lo -MD -MP -MF $depbase.Tpo -c -o
> lib/jhash.lo lib/jhash.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 -MT lib/jhash.lo -MD -MP -MF lib/.deps/jhash.Tpo -c
> lib/jhash.c -o lib/jhash.o depbase=`echo lib/json.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 -MT lib/json.lo -MD -MP -MF $depbase.Tpo -c -o
> lib/json.lo lib/json.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 -MT lib/json.lo -MD -MP -MF lib/.deps/json.Tpo -c
> lib/json.c -o lib/json.o depbase=`echo lib/jsonrpc.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 -MT lib/jsonrpc.lo -MD -MP -MF $depbase.Tpo -c -o
> lib/jsonrpc.lo lib/jsonrpc.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 -MT lib/jsonrpc.lo -MD -MP -MF lib/.deps/jsonrpc.Tpo -c
> lib/jsonrpc.c -o lib/jsonrpc.o depbase=`echo lib/lacp.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 -MT lib/lacp.lo -MD -MP -MF $depbase.Tpo -c -o
> lib/lacp.lo lib/lacp.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 -MT lib/lacp.lo -MD -MP -MF lib/.deps/lacp.Tpo -c
> lib/lacp.c -o lib/lacp.o
> lib/lacp.c: In function 'lacp_process_packet':
> lib/lacp.c:329:52: error: unused parameter 'bond'
> [-Werror=unused-parameter] lacp_process_packet(struct lacp *lacp,
> const void *bond, const void *slave_,
>                                                     ^
> cc1: all warnings being treated as errors
> make[2]: *** [lib/lacp.lo] Error 1
> make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
> make[1]: *** [all-recursive] Error 1
> make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
> make: *** [all] Error 2
>
>
> Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
>
> Thanks,
> 0-day Robot
Ben Pfaff Feb. 27, 2019, 6:14 p.m. UTC | #3
This series adds a warning:

    ../lib/lacp.c:360:47: error: passing 'const void *' to parameter of type 'void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
    ../ofproto/bond.h:83:60: note: passing argument to parameter 'slave_' here

Please fix it.  Thanks!
Aaron Conole March 6, 2019, 2:58 p.m. UTC | #4
Aaron Conole <aconole@bytheb.org> writes:

> Nitin Katiyar <nitin.katiyar@ericsson.com> writes:
>
>> Hi,
>> I got this mail reporting build failure. It complains for bond
>> variable not being used. I had sent patch earlier after correcting
>> this. So, I am not sure which patch version it is complaining for. In
>> my latest patch I see that it is being used. Am I missing something?
>
> Looking at the message ID to which you're replying:
>
>   Robot mail: 1551257882-9544-1-git-send-email-robot@bytheb.org
>
> Looking at that message id, it is a reply to:
>
>   Nitin Katiyar: 1551285539-31225-1-git-send-email-nitin.katiyar@ericsson.com
>
> Looking at that message, I see that it includes:
>
> @@ -324,8 +325,8 @@ lacp_is_active(const struct lacp *lacp) OVS_EXCLUDED(mutex)
>  /* Processes 'packet' which was received on 'slave_'.  This function should be
>   * called on all packets received on 'slave_' with Ethernet Type ETH_TYPE_LACP.
>   */
> -void
> -lacp_process_packet(struct lacp *lacp, const void *slave_,
> +bool
> +lacp_process_packet(struct lacp *lacp, const void *bond, const void *slave_,
>                      const struct dp_packet *packet)
>      OVS_EXCLUDED(mutex)
>  {
>
>
> Which indeed looks like the bond argument is added, but never referenced.
>
>> On other hand, it will be good if report captures the patch or version of the patch.
>
> I agree, I'll fix that up.

Just following up.  This should be fixed now.

> On the other hand, the email in question is at url:
>
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356740.html
>
> It does not contain a version string.  So it seems this is a valid
> report.
>
>> Regards,
>> Nitin
>>
>> -----Original Message-----
>> From: 0-day Robot [mailto:robot@bytheb.org] 
>> Sent: Wednesday, February 27, 2019 2:28 PM
>> To: Nitin Katiyar <nitin.katiyar@ericsson.com>
>> Cc: dev@openvswitch.org
>> Subject: Re: Avoid packet drop on LACP bond after link up
>>
>> Bleep bloop.  Greetings Nitin Katiyar, 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 -MT lib/jhash.lo -MD -MP -MF $depbase.Tpo -c -o
>> lib/jhash.lo lib/jhash.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 -MT lib/jhash.lo -MD -MP -MF lib/.deps/jhash.Tpo -c
>> lib/jhash.c -o lib/jhash.o depbase=`echo lib/json.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 -MT lib/json.lo -MD -MP -MF $depbase.Tpo -c -o
>> lib/json.lo lib/json.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 -MT lib/json.lo -MD -MP -MF lib/.deps/json.Tpo -c
>> lib/json.c -o lib/json.o depbase=`echo lib/jsonrpc.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 -MT lib/jsonrpc.lo -MD -MP -MF $depbase.Tpo -c -o
>> lib/jsonrpc.lo lib/jsonrpc.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 -MT lib/jsonrpc.lo -MD -MP -MF lib/.deps/jsonrpc.Tpo -c
>> lib/jsonrpc.c -o lib/jsonrpc.o depbase=`echo lib/lacp.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 -MT lib/lacp.lo -MD -MP -MF $depbase.Tpo -c -o
>> lib/lacp.lo lib/lacp.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 -MT lib/lacp.lo -MD -MP -MF lib/.deps/lacp.Tpo -c
>> lib/lacp.c -o lib/lacp.o
>> lib/lacp.c: In function 'lacp_process_packet':
>> lib/lacp.c:329:52: error: unused parameter 'bond'
>> [-Werror=unused-parameter] lacp_process_packet(struct lacp *lacp,
>> const void *bond, const void *slave_,
>>                                                     ^
>> cc1: all warnings being treated as errors
>> make[2]: *** [lib/lacp.lo] Error 1
>> make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
>> make[1]: *** [all-recursive] Error 1
>> make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
>> make: *** [all] Error 2
>>
>>
>> Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
>>
>> Thanks,
>> 0-day Robot

Patch
diff mbox series

========
During port DOWN->UP of link (slave) in a LACP bond, after receiving the
LACPDU with SYNC set for both actor and partner, the bond-slave remains
"disabled" until OVS main thread runs LACP state machine and eventually
"enables" the bond-slave. With this, we have observed delays in the order
of 350ms and packets are  dropped in OVS due to bond-admissibility check
(packets received on slave in "disabled" state are dropped).

Fix:
====
When a LACPDU is received, evaluate whether LACP slave can be enabled
(slave_may_enable()) and set LACP slave's may_enable from the datapath
thread itself. When may_enable = TRUE, it means L1 state is UP and
LACP-SYNC is done and it is waiting for the main thread to enable the
slave. Relax the check in bond_check_admissibility() to check for both
"enable" and "may_enable" of the LACP slave. This would avoid dropping
of packets until the main thread enables the slave from bundle_run().

Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com>
Signed-off-by: Manohar Krishnappa Chidambaraswamy <manukc@gmail.com>
Co-authored-by: Manohar Krishnappa Chidambaraswamy <manukc@gmail.com>
---
 lib/lacp.c                   | 12 ++++++++++--
 lib/lacp.h                   |  3 ++-
 ofproto/bond.c               | 16 +++++++++++++---
 ofproto/ofproto-dpif-xlate.c | 11 ++++++++++-
 4 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/lib/lacp.c b/lib/lacp.c
index d6b36aa..7ac85f9 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -154,6 +154,7 @@  static struct slave *slave_lookup(const struct lacp *, const void *slave)
     OVS_REQUIRES(mutex);
 static bool info_tx_equal(struct lacp_info *, struct lacp_info *)
     OVS_REQUIRES(mutex);
+static bool slave_may_enable__(struct slave *slave) OVS_REQUIRES(mutex);
 
 static unixctl_cb_func lacp_unixctl_show;
 static unixctl_cb_func lacp_unixctl_show_stats;
@@ -324,8 +325,8 @@  lacp_is_active(const struct lacp *lacp) OVS_EXCLUDED(mutex)
 /* Processes 'packet' which was received on 'slave_'.  This function should be
  * called on all packets received on 'slave_' with Ethernet Type ETH_TYPE_LACP.
  */
-void
-lacp_process_packet(struct lacp *lacp, const void *slave_,
+bool
+lacp_process_packet(struct lacp *lacp, const void *bond, const void *slave_,
                     const struct dp_packet *packet)
     OVS_EXCLUDED(mutex)
 {
@@ -333,6 +334,7 @@  lacp_process_packet(struct lacp *lacp, const void *slave_,
     const struct lacp_pdu *pdu;
     long long int tx_rate;
     struct slave *slave;
+    bool lacp_may_enable = false;
 
     lacp_lock();
     slave = slave_lookup(lacp, slave_);
@@ -362,8 +364,14 @@  lacp_process_packet(struct lacp *lacp, const void *slave_,
         slave->partner = pdu->actor;
     }
 
+    /* Evaluate may_enable here to avoid dropping of packets till main thread
+     * sets may_enable to true. */
+    lacp_may_enable = slave_may_enable__(slave);
+
 out:
     lacp_unlock();
+
+    return lacp_may_enable;
 }
 
 /* Returns the lacp_status of the given 'lacp' object (which may be NULL). */
diff --git a/lib/lacp.h b/lib/lacp.h
index f35cff5..1505c2c 100644
--- a/lib/lacp.h
+++ b/lib/lacp.h
@@ -46,7 +46,8 @@  struct lacp *lacp_ref(const struct lacp *);
 void lacp_configure(struct lacp *, const struct lacp_settings *);
 bool lacp_is_active(const struct lacp *);
 
-void lacp_process_packet(struct lacp *, const void *slave,
+bool lacp_process_packet(struct lacp *, const void *bond,
+                         const void *slave,
                          const struct dp_packet *packet);
 enum lacp_status lacp_status(const struct lacp *);
 
diff --git a/ofproto/bond.c b/ofproto/bond.c
index d2a8b1f..cd6fd33 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -794,6 +794,7 @@  bond_check_admissibility(struct bond *bond, const void *slave_,
 {
     enum bond_verdict verdict = BV_DROP;
     struct bond_slave *slave;
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 
     ovs_rwlock_rdlock(&rwlock);
     slave = bond_slave_lookup(bond, slave_);
@@ -811,7 +812,11 @@  bond_check_admissibility(struct bond *bond, const void *slave_,
      * drop all incoming traffic except if lacp_fallback_ab is enabled. */
     switch (bond->lacp_status) {
     case LACP_NEGOTIATED:
-        verdict = slave->enabled ? BV_ACCEPT : BV_DROP;
+        /* To reduce packet-drops due to delay in enabling of slave (post
+         * LACP-SYNC), from main thread, check for may_enable as well.
+         * When may_enable is TRUE, it means LACP is UP and waiting for the
+         * main thread to run LACP state machine and enable the slave. */
+        verdict = (slave->enabled || slave->may_enable) ? BV_ACCEPT : BV_DROP;
         goto out;
     case LACP_CONFIGURED:
         if (!bond->lacp_fallback_ab) {
@@ -847,8 +852,6 @@  bond_check_admissibility(struct bond *bond, const void *slave_,
         /* Drop all packets which arrive on backup slaves.  This is similar to
          * how Linux bonding handles active-backup bonds. */
         if (bond->active_slave != slave) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-
             VLOG_DBG_RL(&rl, "active-backup bond received packet on backup"
                         " slave (%s) destined for " ETH_ADDR_FMT,
                         slave->name, ETH_ADDR_ARGS(eth_dst));
@@ -870,6 +873,13 @@  bond_check_admissibility(struct bond *bond, const void *slave_,
 
     OVS_NOT_REACHED();
 out:
+    if (slave && (verdict != BV_ACCEPT)) {
+        VLOG_DBG_RL(&rl, "slave= %s actv-slave= %d may_enable %d enable %d "
+                    "LACP %d verdict(A/D/M=0/1/2) %d\n", slave->name,
+                    (bond->active_slave == slave), slave->may_enable,
+                    slave->enabled, bond->lacp_status, verdict);
+    }
+
     ovs_rwlock_unlock(&rwlock);
     return verdict;
 
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index acd4817..a4a3a2b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3288,6 +3288,7 @@  process_special(struct xlate_ctx *ctx, const struct xport *xport)
     const struct xbridge *xbridge = ctx->xbridge;
     const struct dp_packet *packet = ctx->xin->packet;
     enum slow_path_reason slow;
+    bool lacp_may_enable;
 
     if (!xport) {
         slow = 0;
@@ -3308,7 +3309,15 @@  process_special(struct xlate_ctx *ctx, const struct xport *xport)
     } else if (xport->xbundle && xport->xbundle->lacp
                && flow->dl_type == htons(ETH_TYPE_LACP)) {
         if (packet) {
-            lacp_process_packet(xport->xbundle->lacp, xport->ofport, packet);
+            lacp_may_enable = lacp_process_packet(xport->xbundle->lacp,
+                                                  xport->xbundle->bond,
+                                                  xport->ofport, packet);
+            /* Update LACP status in bond-slave to avoid packet-drops until
+             * LACP state machine is run by the main thread. */
+            if (xport->xbundle->bond && lacp_may_enable) {
+                bond_slave_set_may_enable(xport->xbundle->bond, xport->ofport,
+                                          lacp_may_enable);
+            }
         }
         slow = SLOW_LACP;
     } else if ((xbridge->stp || xbridge->rstp) &&