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: > > On 2/13/19 10:03 AM, Randall Stewart wrote: >> oh and one other thing.. >> >> It was *not* a random IFP.. it was the IFP to the lagg. >> >> I.e. an alloc() was done to the lagg.. and the free was >> done back to the same IFP (that provided the allocate). > > 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. > > 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’s 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’t see how it would panic. R > >> R >> >>> On Feb 13, 2019, at 1:02 PM, Randall Stewart <rrs@netflix.com> wrote: >>> >>> I disagree. If you define an alloc it is only >>> reciprocal that you should define a free. >>> >>> 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… >>> >>> R >>> >>> >>>> On Feb 13, 2019, at 12:09 PM, John Baldwin <jhb@freebsd.org> wrote: >>>> >>>> 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 >>>>> >>>>> 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). >>>>> >>>>> Sponsored by: Netflix Inc. >>>>> Differential Revision: https://reviews.freebsd.org/D19032 >>>> >>>> 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. >>>> >>>> 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. >>>> >>>> -- >>>> John Baldwin >>>> >>>> >>> >>> ------ >>> Randall Stewart >>> rrs@netflix.com >>> >>> >>> >> >> ------ >> Randall Stewart >> rrs@netflix.com >> >> >> > > > -- > 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>