> On Feb 13, 2019, at 1:10 PM, John Baldwin <j...@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 <r...@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 <j...@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 >>> r...@netflix.com >>> >>> >>> >> >> ------ >> Randall Stewart >> r...@netflix.com >> >> >> > > > -- > John Baldwin ------ Randall Stewart r...@netflix.com _______________________________________________ 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"