Date: Fri, 20 Jan 2017 18:37:20 -0800 From: Ravi Pokala <rpokala@mac.com> To: Hiren Panchasara <hiren@freebsd.org>, Alan Somers <asomers@freebsd.org>, lakshmi.n@msystechnologies.com, smh@FreeBSD.org Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r286700 - in head: sbin/ifconfig sys/net Message-ID: <D199ED54-1E24-46AB-8541-DC6AD5C55B57@panasas.com> In-Reply-To: <20170118223827.GJ86256@strugglingcoder.info> References: <201508122021.t7CKL5wk016750@repo.freebsd.org> <CAOtMX2jwU-mVT-6F7=y5aS8i=VBjya%2BU_9r5r%2BJ=tLK4Z518VA@mail.gmail.com> <20170118223827.GJ86256@strugglingcoder.info>
next in thread | previous in thread | raw e-mail | index | archive | help
-----Original Message----- > From: <owner-src-committers@freebsd.org> on behalf of Hiren Panchasara <hiren@freebsd.org> > Date: 2017-01-18, Wednesday at 14:38 > To: Alan Somers <asomers@freebsd.org>, <lakshmi.n@msystechnologies.com>, <rpokala@FreeBSD.org>, <smh@FreeBSD.org> > Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> > Subject: Re: svn commit: r286700 - in head: sbin/ifconfig sys/net > > Adding the submitter and other reviewers for their comments. > > On 01/18/17 at 03:03P, Alan Somers wrote: >> Is the change to lacp_port_create correct? The comment indicates that >> fast is configurable, but it's actually constant. Later on, there's >> some dead code that depends on the value of fast (it was dead before >> this commit, too). >> >> CID: 1305734 >> CID: 1305692 You're right that in lacp_port_create(), "fast" (and "active") are both constant. I think the comment intended to indicate that "fast" could be changed via ioctl *after create*. It seems to me that the right thing to do would be to remove both "fast" and "active" from lacp_port_create(), and have it unconditionally set "lp->lp_state" to LACP_STATE_ACTIVITY. That would eliminate the unclear comments and fix both Coverity issues, without changing any behavior. -Ravi (rpokala@) >> -Alan >> >> On Wed, Aug 12, 2015 at 2:21 PM, Hiren Panchasara <hiren@freebsd.org> wrote: >>> Author: hiren >>> Date: Wed Aug 12 20:21:04 2015 >>> New Revision: 286700 >>> URL: https://svnweb.freebsd.org/changeset/base/286700 >>> >>> Log: >>> Make LAG LACP fast timeout tunable through IOCTL. >>> >>> Differential Revision: D3300 >>> Submitted by: LN Sundararajan <lakshmi.n at msystechnologies> >>> Reviewed by: wblock, smh, gnn, hiren, rpokala at panasas >>> MFC after: 2 weeks >>> Sponsored by: Panasas >>> >>> Modified: >>> head/sbin/ifconfig/ifconfig.8 >>> head/sbin/ifconfig/iflagg.c >>> head/sys/net/ieee8023ad_lacp.c >>> head/sys/net/ieee8023ad_lacp.h >>> head/sys/net/if_lagg.c >>> head/sys/net/if_lagg.h >>> >>> Modified: head/sbin/ifconfig/ifconfig.8 >>> ============================================================================== >>> --- head/sbin/ifconfig/ifconfig.8 Wed Aug 12 20:16:13 2015 (r286699) >>> +++ head/sbin/ifconfig/ifconfig.8 Wed Aug 12 20:21:04 2015 (r286700) >>> @@ -28,7 +28,7 @@ >>> .\" From: @(#)ifconfig.8 8.3 (Berkeley) 1/5/94 >>> .\" $FreeBSD$ >>> .\" >>> -.Dd May 15, 2015 >>> +.Dd Aug 12, 2015 >>> .Dt IFCONFIG 8 >>> .Os >>> .Sh NAME >>> @@ -2396,6 +2396,10 @@ Disable local hash computation for RSS h >>> Set a shift parameter for RSS local hash computation. >>> Hash is calculated by using flowid bits in a packet header mbuf >>> which are shifted by the number of this parameter. >>> +.It Cm lacp_fast_timeout >>> +Enable lacp fast-timeout on the interface. >>> +.It Cm -lacp_fast_timeout >>> +Disable lacp fast-timeout on the interface. >>> .El >>> .Pp >>> The following parameters are specific to IP tunnel interfaces, >>> >>> Modified: head/sbin/ifconfig/iflagg.c >>> ============================================================================== >>> --- head/sbin/ifconfig/iflagg.c Wed Aug 12 20:16:13 2015 (r286699) >>> +++ head/sbin/ifconfig/iflagg.c Wed Aug 12 20:21:04 2015 (r286700) >>> @@ -115,6 +115,8 @@ setlaggsetopt(const char *val, int d, in >>> case -LAGG_OPT_LACP_TXTEST: >>> case LAGG_OPT_LACP_RXTEST: >>> case -LAGG_OPT_LACP_RXTEST: >>> + case LAGG_OPT_LACP_TIMEOUT: >>> + case -LAGG_OPT_LACP_TIMEOUT: >>> break; >>> default: >>> err(1, "Invalid lagg option"); >>> @@ -293,6 +295,8 @@ static struct cmd lagg_cmds[] = { >>> DEF_CMD("-lacp_txtest", -LAGG_OPT_LACP_TXTEST, setlaggsetopt), >>> DEF_CMD("lacp_rxtest", LAGG_OPT_LACP_RXTEST, setlaggsetopt), >>> DEF_CMD("-lacp_rxtest", -LAGG_OPT_LACP_RXTEST, setlaggsetopt), >>> + DEF_CMD("lacp_fast_timeout", LAGG_OPT_LACP_TIMEOUT, setlaggsetopt), >>> + DEF_CMD("-lacp_fast_timeout", -LAGG_OPT_LACP_TIMEOUT, setlaggsetopt), >>> DEF_CMD_ARG("flowid_shift", setlaggflowidshift), >>> }; >>> static struct afswtch af_lagg = { >>> >>> Modified: head/sys/net/ieee8023ad_lacp.c >>> ============================================================================== >>> --- head/sys/net/ieee8023ad_lacp.c Wed Aug 12 20:16:13 2015 (r286699) >>> +++ head/sys/net/ieee8023ad_lacp.c Wed Aug 12 20:21:04 2015 (r286700) >>> @@ -522,7 +522,7 @@ lacp_port_create(struct lagg_port *lgp) >>> int error; >>> >>> boolean_t active = TRUE; /* XXX should be configurable */ >>> - boolean_t fast = FALSE; /* XXX should be configurable */ >>> + boolean_t fast = FALSE; /* Configurable via ioctl */ >>> >>> link_init_sdl(ifp, (struct sockaddr *)&sdl, IFT_ETHER); >>> sdl.sdl_alen = ETHER_ADDR_LEN; >>> >>> Modified: head/sys/net/ieee8023ad_lacp.h >>> ============================================================================== >>> --- head/sys/net/ieee8023ad_lacp.h Wed Aug 12 20:16:13 2015 (r286699) >>> +++ head/sys/net/ieee8023ad_lacp.h Wed Aug 12 20:21:04 2015 (r286700) >>> @@ -251,6 +251,7 @@ struct lacp_softc { >>> u_int32_t lsc_tx_test; >>> } lsc_debug; >>> u_int32_t lsc_strict_mode; >>> + boolean_t lsc_fast_timeout; /* if set, fast timeout */ >>> }; >>> >>> #define LACP_TYPE_ACTORINFO 1 >>> >>> Modified: head/sys/net/if_lagg.c >>> ============================================================================== >>> --- head/sys/net/if_lagg.c Wed Aug 12 20:16:13 2015 (r286699) >>> +++ head/sys/net/if_lagg.c Wed Aug 12 20:21:04 2015 (r286700) >>> @@ -1257,6 +1257,8 @@ lagg_ioctl(struct ifnet *ifp, u_long cmd >>> ro->ro_opts |= LAGG_OPT_LACP_RXTEST; >>> if (lsc->lsc_strict_mode != 0) >>> ro->ro_opts |= LAGG_OPT_LACP_STRICT; >>> + if (lsc->lsc_fast_timeout != 0) >>> + ro->ro_opts |= LAGG_OPT_LACP_TIMEOUT; >>> >>> ro->ro_active = sc->sc_active; >>> } else { >>> @@ -1292,6 +1294,8 @@ lagg_ioctl(struct ifnet *ifp, u_long cmd >>> case -LAGG_OPT_LACP_RXTEST: >>> case LAGG_OPT_LACP_STRICT: >>> case -LAGG_OPT_LACP_STRICT: >>> + case LAGG_OPT_LACP_TIMEOUT: >>> + case -LAGG_OPT_LACP_TIMEOUT: >>> valid = lacp = 1; >>> break; >>> default: >>> @@ -1320,6 +1324,7 @@ lagg_ioctl(struct ifnet *ifp, u_long cmd >>> sc->sc_opts &= ~ro->ro_opts; >>> } else { >>> struct lacp_softc *lsc; >>> + struct lacp_port *lp; >>> >>> lsc = (struct lacp_softc *)sc->sc_psc; >>> >>> @@ -1342,6 +1347,20 @@ lagg_ioctl(struct ifnet *ifp, u_long cmd >>> case -LAGG_OPT_LACP_STRICT: >>> lsc->lsc_strict_mode = 0; >>> break; >>> + case LAGG_OPT_LACP_TIMEOUT: >>> + LACP_LOCK(lsc); >>> + LIST_FOREACH(lp, &lsc->lsc_ports, lp_next) >>> + lp->lp_state |= LACP_STATE_TIMEOUT; >>> + LACP_UNLOCK(lsc); >>> + lsc->lsc_fast_timeout = 1; >>> + break; >>> + case -LAGG_OPT_LACP_TIMEOUT: >>> + LACP_LOCK(lsc); >>> + LIST_FOREACH(lp, &lsc->lsc_ports, lp_next) >>> + lp->lp_state &= ~LACP_STATE_TIMEOUT; >>> + LACP_UNLOCK(lsc); >>> + lsc->lsc_fast_timeout = 0; >>> + break; >>> } >>> } >>> LAGG_WUNLOCK(sc); >>> >>> Modified: head/sys/net/if_lagg.h >>> ============================================================================== >>> --- head/sys/net/if_lagg.h Wed Aug 12 20:16:13 2015 (r286699) >>> +++ head/sys/net/if_lagg.h Wed Aug 12 20:21:04 2015 (r286700) >>> @@ -150,6 +150,7 @@ struct lagg_reqopts { >>> #define LAGG_OPT_LACP_STRICT 0x10 /* LACP strict mode */ >>> #define LAGG_OPT_LACP_TXTEST 0x20 /* LACP debug: txtest */ >>> #define LAGG_OPT_LACP_RXTEST 0x40 /* LACP debug: rxtest */ >>> +#define LAGG_OPT_LACP_TIMEOUT 0x80 /* LACP timeout */ >>> u_int ro_count; /* number of ports */ >>> u_int ro_active; /* active port count */ >>> u_int ro_flapping; /* number of flapping */ >>>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D199ED54-1E24-46AB-8541-DC6AD5C55B57>