Re: svn commit: r344099 - head/sys/net
On 2/22/19 8:50 AM, Andrew Gallatin wrote: > I think the misunderstanding here is that I think he's not getting the > ifp from the route. > > My recollection is that he is holding the ifps when he enables HW pacing > in BBR. Due to limitations in different NIC hardware, you can only have > N different rates, etc. So he goes ahead and allocates those N rates up > front so that he knows he can reserve them & know that he can always get > them. > > Then when the system reboots, BBR has an eventhandler that goes ahead > and frees those reservations. I think that he's using the ifp that he's > holding here. > > In the case that tripped him up, that ifp was lagg. > > Your workaround would also work, but Randall does have a point about > symmetric alloc/free especially when viewed from his perspective, But it's not really an alloc. We have many other places in the kernel where an alloc routine is actually a "forward" to something else that allocates the real thing. For example, when you add a kevent on a file descriptor, that is routed to the 'file_filtops' in kern/kern_event.c, but that filtops only has an attach routine, it doesn't have a detach routine because it's attach routine is a forwarder to other filtops. It invokes the fo_kqfilter method that is responsible for setting up the right filtops to use. For sockets this routes to one of three filtops that all have detach methods but no attach methods. The way this is handled is that anytime a kevent is detached, the current filtops is used to find the detach event handler. We don't use the filter's type to get back to the original file_filtops and require that to forward the detach request on to the real filtops. Having the tag save the "real" ifp is similar to knote's storing the real filtops. Given your specific use case, it does seem that this commit is sufficient, but it's not the "always works" way to free a tag, and it might be used as a template that gets copied for use in another case where the extra guarantees of this specific use case don't hold. Given that I'd like to explore making lagg and vlan allocate real interposing tags to handle TLS, this may prove mostly academic as they would have "real" free routines then since their alloc routines would just be forwarders. -- John Baldwin ___ 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"
Re: svn commit: r344099 - head/sys/net
I think the misunderstanding here is that I think he's not getting the ifp from the route. My recollection is that he is holding the ifps when he enables HW pacing in BBR. Due to limitations in different NIC hardware, you can only have N different rates, etc. So he goes ahead and allocates those N rates up front so that he knows he can reserve them & know that he can always get them. Then when the system reboots, BBR has an eventhandler that goes ahead and frees those reservations. I think that he's using the ifp that he's holding here. In the case that tripped him up, that ifp was lagg. Your workaround would also work, but Randall does have a point about symmetric alloc/free especially when viewed from his perspective, Drew ___ 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"
Re: svn commit: r344099 - head/sys/net
Hi,
On 2/21/19 7:28 PM, John Baldwin wrote:
On 2/21/19 7:29 AM, Randall Stewart wrote:
On Feb 13, 2019, at 1:10 PM, John Baldwin 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.
My point is that your calling code should do this.
Suppose I have a socket over cc0. It allocates a send tag. Later, the
connection is rerouted over em0 and em(4) doesn't support send tags at all.
If you use the ifp from the route to free the tag, you will try to use
em's if_snd_tag_free routine and that will be NULL and panic. The fix is
that you shouldn't be using the route ifp to free the tag, only to note that
the tag is stale, so something like:
ifp = route.rt_ifp;
if (ifp != m->m_snd_tag->ifp) {
/* Need to free tag. */
tag = m->m_snd_tag;
/* Free mbuf or clear tag or whatever */
tag->ifp->if_snd_tag_free(tag);
}
if_snd_tag_free should always be called from 'tag->ifp' as that's the ifp
that owns that tag and knows how to free it.
Your patch happens to work for the case of the route going over a lagg and
the connection switching between lagg ports only because you've made the
lagg routine do what the calling code should be doing in the first place.
I haven't seen Randall's code, but unless he is using the same ifp to
free the send that that was used to allocate it, then John is right. The
send tag already contains an interface pointer, that should be used when
freeing it.
--HPS
___
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"
Re: svn commit: r344099 - head/sys/net
On 2/21/19 7:29 AM, Randall Stewart wrote:
>
>
>> On Feb 13, 2019, at 1:10 PM, John Baldwin 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.
My point is that your calling code should do this.
Suppose I have a socket over cc0. It allocates a send tag. Later, the
connection is rerouted over em0 and em(4) doesn't support send tags at all.
If you use the ifp from the route to free the tag, you will try to use
em's if_snd_tag_free routine and that will be NULL and panic. The fix is
that you shouldn't be using the route ifp to free the tag, only to note that
the tag is stale, so something like:
ifp = route.rt_ifp;
if (ifp != m->m_snd_tag->ifp) {
/* Need to free tag. */
tag = m->m_snd_tag;
/* Free mbuf or clear tag or whatever */
tag->ifp->if_snd_tag_free(tag);
}
if_snd_tag_free should always be called from 'tag->ifp' as that's the ifp
that owns that tag and knows how to free it.
Your patch happens to work for the case of the route going over a lagg and
the connection switching between lagg ports only because you've made the
lagg routine do what the calling code should be doing in the first place.
--
John Baldwin
___
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"
Re: svn commit: r344099 - head/sys/net
> On Feb 13, 2019, at 1:10 PM, John Baldwin 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 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 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"
Re: svn commit: r344099 - head/sys/net
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. > R > >> On Feb 13, 2019, at 1:02 PM, Randall Stewart 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 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 ___ 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"
Re: svn commit: r344099 - head/sys/net
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). R > On Feb 13, 2019, at 1:02 PM, Randall Stewart 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 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 ___ 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"
Re: svn commit: r344099 - head/sys/net
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 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 ___ 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"
Re: svn commit: r344099 - head/sys/net
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 ___ 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"
