Date: Thu, 21 Feb 2019 10:29:46 -0500 From: Randall Stewart <rrs@netflix.com> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r344099 - head/sys/net Message-ID: <E0A0F0A9-18E2-47B0-9BB3-CF90C666ADA4@netflix.com> In-Reply-To: <89d15ffe-1bc9-adaf-9307-4bf6541cc5e1@FreeBSD.org> References: <201902131457.x1DEvx9V051533@repo.freebsd.org> <99453977-0f52-6050-3f40-e0fd7ea43d7f@FreeBSD.org> <80314D46-6FEC-462D-8EC5-FCE1ECFF81EF@netflix.com> <DC8566A0-7C0E-41E9-AA6A-7CDCE58976B4@netflix.com> <89d15ffe-1bc9-adaf-9307-4bf6541cc5e1@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Feb 13, 2019, at 1:10 PM, John Baldwin <jhb@freebsd.org> wrote: >=20 > On 2/13/19 10:03 AM, Randall Stewart wrote: >> oh and one other thing.. >>=20 >> It was *not* a random IFP.. it was the IFP to the lagg. >>=20 >> I.e. an alloc() was done to the lagg.. and the free was >> done back to the same IFP (that provided the allocate). >=20 > Yes, that's wrong. Suppose the route changes so that my traffic is = now over > em0 instead of lagg0 (where em0 isn't a member of the lagg), how do = you > expect if_lagg_free to invoke em0's free routine? In your case it = does, > but only by accident. It doesn't work in the other case I described = which > is if you have non-lagg interfaces and a route moves from cc0 to em0. = In > that case your existing code that is using the wrong ifp will just = panic. >=20 > These aren't real alloc routines as the lagg and vlan ones don't = allocate > anything, they pass along the request to the child and the child = allocates > the tag. Only ifnet's that actually allocate tags should need to free = them, > and you should be using tag->ifp to as the ifp whose if_snd_tag_free = works. But thats what the lagg=E2=80=99s routine does, use the tag sent in to find the real ifp (where the tag was allocated) and call the if_snd_tag_free() on that. Its not an accident it works, it calls the free of the actual interface where the allocation came from. I don=E2=80=99t see how it would panic. R >=20 >> R >>=20 >>> On Feb 13, 2019, at 1:02 PM, Randall Stewart <rrs@netflix.com> = wrote: >>>=20 >>> I disagree. If you define an alloc it is only >>> reciprocal that you should define a free. >>>=20 >>> The code in question that hit this was changed (its in a version >>> of rack that has the rate-limit and TLS code).. but I think these >>> things *should* be balanced.. if you provide an Allocate, you >>> should also provide a Free=E2=80=A6=20 >>>=20 >>> R >>>=20 >>>=20 >>>> On Feb 13, 2019, at 12:09 PM, John Baldwin <jhb@freebsd.org> wrote: >>>>=20 >>>> On 2/13/19 6:57 AM, Randall Stewart wrote: >>>>> Author: rrs >>>>> Date: Wed Feb 13 14:57:59 2019 >>>>> New Revision: 344099 >>>>> URL: https://svnweb.freebsd.org/changeset/base/344099 >>>>>=20 >>>>> Log: >>>>> This commit adds the missing release mechanism for the >>>>> ratelimiting code. The two modules (lagg and vlan) did have >>>>> allocation routines, and even though they are indirect (and >>>>> vector down to the underlying interfaces) they both need to >>>>> have a free routine (that also vectors down to the actual = interface). >>>>>=20 >>>>> Sponsored by: Netflix Inc. >>>>> Differential Revision: https://reviews.freebsd.org/D19032 >>>>=20 >>>> Hmm, I don't understand why you'd ever invoke if_snd_tag_free from = anything >>>> but 'tag->ifp' rather than some other ifp. What if the route for a = connection >>>> moves so that a tag allocated on cc0 is now on a route that goes = over em0? >>>> You can't expect em0 to have an if_snd_tag_free routine that will = know to >>>> go invoke cxgbe's snd_tag_free. I think you should always be using >>>> 'tag->ifp->if_snd_tag_free' to free tags and never using any other = ifp. >>>>=20 >>>> That is, I think this should be reverted and that instead you need = to fix >>>> the code invoking if_snd_tag_free to invoke it on the tag's ifp = instead of >>>> some random other ifp. >>>>=20 >>>> --=20 >>>> John Baldwin >>>>=20 >>>>=20 >>>=20 >>> ------ >>> Randall Stewart >>> rrs@netflix.com >>>=20 >>>=20 >>>=20 >>=20 >> ------ >> Randall Stewart >> rrs@netflix.com >>=20 >>=20 >>=20 >=20 >=20 > --=20 > John Baldwin ------ Randall Stewart rrs@netflix.com
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?E0A0F0A9-18E2-47B0-9BB3-CF90C666ADA4>