-----Original Message----- > From: <owner-src-committ...@freebsd.org> on behalf of Hiren Panchasara > <hi...@freebsd.org> > Date: 2017-01-18, Wednesday at 14:38 > To: Alan Somers <asom...@freebsd.org>, <lakshm...@msystechnologies.com>, > <rpok...@freebsd.org>, <s...@freebsd.org> > Cc: "src-committ...@freebsd.org" <src-committ...@freebsd.org>, > "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, > "svn-src-h...@freebsd.org" <svn-src-h...@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 <hi...@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 */ >>> _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
