Date: Mon, 23 Jan 2017 10:12:14 +0530 From: Lakshmi Narasimhan Sundararajan <lakshmi.n@msystechnologies.com> To: Ravi Pokala <rpokala@mac.com> Cc: Hiren Panchasara <hiren@freebsd.org>, Alan Somers <asomers@freebsd.org>, smh@freebsd.org, "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: <CA%2BNkpgiEFaeL8vQ8KtHp-xiw8GgPFwMJSCTRd7vQ2%2BsmS8YgEg@mail.gmail.com> In-Reply-To: <D199ED54-1E24-46AB-8541-DC6AD5C55B57@panasas.com> References: <201508122021.t7CKL5wk016750@repo.freebsd.org> <CAOtMX2jwU-mVT-6F7=y5aS8i=VBjya%2BU_9r5r%2BJ=tLK4Z518VA@mail.gmail.com> <20170118223827.GJ86256@strugglingcoder.info> <D199ED54-1E24-46AB-8541-DC6AD5C55B57@panasas.com>
next in thread | previous in thread | raw e-mail | index | archive | help
It's been a long while since we looked at this. We have moved along to other projects. I am sorry, I cannot be of much help now. Regards LN On Sat, Jan 21, 2017 at 8:07 AM, Ravi Pokala <rpokala@mac.com> wrote: > -----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 */ > >>> > > > > -- DISCLAIMER The information in this e-mail is confidential and may be subject to legal privilege. It is intended solely for the addressee. Access to this e-mail by anyone else is unauthorized. If you have received this communication in error, please address with the subject heading "Received in error," send to it@msystechnologies.com, then delete the e-mail and destroy any copies of it. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful. The views, opinions, conclusions and other information expressed in this electronic mail and any attachments are not given or endorsed by the company unless otherwise indicated by an authorized representative independent of this message. MSys cannot guarantee that e-mail communications are secure or error-free, as information could be intercepted, corrupted, amended, lost, destroyed, arrive late or incomplete, or contain viruses, though all reasonable precautions have been taken to ensure no viruses are present in this e-mail. As our company cannot accept responsibility for any loss or damage arising from the use of this e-mail or attachments we recommend that you subject these to your virus checking procedures prior to use
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CA%2BNkpgiEFaeL8vQ8KtHp-xiw8GgPFwMJSCTRd7vQ2%2BsmS8YgEg>