Re: [ovs-discuss] ofproto-dpif-upcall: reg. udpif_revalidator thread
> > > > Vishal, will you send a patch to do that? > > > > Thanks Ben and Ethan for reviewing. I will send a patch to fix this on > dev-list. > Hi Ben, Ethan, I have sent a patch fixing this issue on mailing list for review. Warm Regards, Vishal ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
Re: [ovs-discuss] ofproto-dpif-upcall: reg. udpif_revalidator thread
> > Thanks. After staring at it for a while, I think I'm comfortable with > removing it. > > Vishal, will you send a patch to do that? > Thanks Ben and Ethan for reviewing. I will send a patch to fix this on dev-list. ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
Re: [ovs-discuss] ofproto-dpif-upcall: reg. udpif_revalidator thread
Thanks. After staring at it for a while, I think I'm comfortable with removing it. Vishal, will you send a patch to do that? On Mon, Jul 09, 2018 at 02:43:24AM +, Ethan J. Jackson wrote: > Gosh, this is some old code. On my read, the `n_flows > 2000` is redundant. > My best guess is the line below where it sets a floor on the flow_limit of > 1000 was supposed to catch it. Really that floor should either be 2000, or > the check in the if statement should be `n_flows > 1000`. > > All of that said, I really don't see the point of the `n_flows > 2000` check > at all. I'm a bit worried there was some reason for it that I can't quite > remember, but if I was fixing this myself, I would just remove it., > > Ethan J. Jackson > ejj.sh ( http://ejj.sh ) > > On Fri, Jul 06, 2018 at 10:52 AM, Ben Pfaff < b...@ovn.org > wrote: > > > > > > > > > On Fri, Jul 06, 2018 at 05:22:55PM +, Vishal Deep Ajmera wrote: > > > > > >> > >>> > >>> > >>> At first glance it looks like you're correct, but this code was added in > >>> 2013 with the following commit and hasn't changed since, so I wonder > >>> whether we're missing something important. Ethan, you wrote this code, do > >>> you have any thoughts? > >>> > >>> > >>> > >>> commit e79a6c833e0d72370951d6f8841098103cbb0b2d > >>> Author: Ethan J. Jackson < ejj@ eecs. berkeley. edu ( > >>> e...@eecs.berkeley.edu > >>> ) > Tue Sep 24 13:39:56 2013 > >>> Committer: Ethan J. Jackson < ejj@ eecs. berkeley. edu ( > >>> e...@eecs.berkeley.edu ) > Thu Dec 19 13:27:23 2013 > >>> > >>> > >>> > >>> ofproto: Handle flow installation and eviction in upcall. > >>> > >>> > >>> > >>> This patch moves flow installation and eviction from ofproto-dpif and the > >>> main thread, into ofproto-dpif-upcall. This performs significantly better > >>> (approximately 2x TCP_CRR improvement), and allows ovs-vswitchd to > >>> maintain significantly larger datapath flow tables. On top of that, it > >>> significantly simplifies the code, retiring "struct facet" and friends. > >>> > >>> > >>> > >>> Signed-off-by: Ethan Jackson < ethan@ nicira. com ( et...@nicira.com ) > > >>> Acked-by: Ben Pfaff < blp@ nicira. com ( b...@nicira.com ) > > >>> > >>> > >> > >> > >> > >> Thanks Ben for looking into this issue. Should I send a formal patch for > >> review by removing this check (n_flows > 2000) ? > >> > >> > > > > > > > > Let's see what Ethan has to say first. > > > > > > ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
Re: [ovs-discuss] ofproto-dpif-upcall: reg. udpif_revalidator thread
Gosh, this is some old code. On my read, the `n_flows > 2000` is redundant. My best guess is the line below where it sets a floor on the flow_limit of 1000 was supposed to catch it. Really that floor should either be 2000, or the check in the if statement should be `n_flows > 1000`. All of that said, I really don't see the point of the `n_flows > 2000` check at all. I'm a bit worried there was some reason for it that I can't quite remember, but if I was fixing this myself, I would just remove it., Ethan J. Jackson ejj.sh ( http://ejj.sh ) On Fri, Jul 06, 2018 at 10:52 AM, Ben Pfaff < b...@ovn.org > wrote: > > > > On Fri, Jul 06, 2018 at 05:22:55PM +, Vishal Deep Ajmera wrote: > > >> >>> >>> >>> At first glance it looks like you're correct, but this code was added in >>> 2013 with the following commit and hasn't changed since, so I wonder >>> whether we're missing something important. Ethan, you wrote this code, do >>> you have any thoughts? >>> >>> >>> >>> commit e79a6c833e0d72370951d6f8841098103cbb0b2d >>> Author: Ethan J. Jackson < ejj@ eecs. berkeley. edu ( e...@eecs.berkeley.edu >>> ) > Tue Sep 24 13:39:56 2013 >>> Committer: Ethan J. Jackson < ejj@ eecs. berkeley. edu ( >>> e...@eecs.berkeley.edu ) > Thu Dec 19 13:27:23 2013 >>> >>> >>> >>> ofproto: Handle flow installation and eviction in upcall. >>> >>> >>> >>> This patch moves flow installation and eviction from ofproto-dpif and the >>> main thread, into ofproto-dpif-upcall. This performs significantly better >>> (approximately 2x TCP_CRR improvement), and allows ovs-vswitchd to >>> maintain significantly larger datapath flow tables. On top of that, it >>> significantly simplifies the code, retiring "struct facet" and friends. >>> >>> >>> >>> Signed-off-by: Ethan Jackson < ethan@ nicira. com ( et...@nicira.com ) > >>> Acked-by: Ben Pfaff < blp@ nicira. com ( b...@nicira.com ) > >>> >>> >> >> >> >> Thanks Ben for looking into this issue. Should I send a formal patch for >> review by removing this check (n_flows > 2000) ? >> >> > > > > Let's see what Ethan has to say first. > > >___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
Re: [ovs-discuss] ofproto-dpif-upcall: reg. udpif_revalidator thread
On Fri, Jul 06, 2018 at 05:22:55PM +, Vishal Deep Ajmera wrote: > > > > At first glance it looks like you're correct, but this code was added in > > 2013 with the following commit and hasn't changed since, so I wonder whether > > we're missing something important. Ethan, you wrote this code, do you have > > any thoughts? > > > > commit e79a6c833e0d72370951d6f8841098103cbb0b2d > > Author: Ethan J. Jackson Tue Sep 24 13:39:56 > > 2013 > > Committer: Ethan J. Jackson Thu Dec 19 > > 13:27:23 > > 2013 > > > > ofproto: Handle flow installation and eviction in upcall. > > > > This patch moves flow installation and eviction from ofproto-dpif and > > the main thread, into ofproto-dpif-upcall. This performs > > significantly better (approximately 2x TCP_CRR improvement), and > > allows ovs-vswitchd to maintain significantly larger datapath flow > > tables. On top of that, it significantly simplifies the code, > > retiring "struct facet" and friends. > > > > Signed-off-by: Ethan Jackson > > Acked-by: Ben Pfaff > > Thanks Ben for looking into this issue. Should I send a formal patch > for review by removing this check (n_flows > 2000) ? Let's see what Ethan has to say first. ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
Re: [ovs-discuss] ofproto-dpif-upcall: reg. udpif_revalidator thread
> > At first glance it looks like you're correct, but this code was added in > 2013 with the following commit and hasn't changed since, so I wonder whether > we're missing something important. Ethan, you wrote this code, do you have > any thoughts? > > commit e79a6c833e0d72370951d6f8841098103cbb0b2d > Author: Ethan J. Jackson Tue Sep 24 13:39:56 > 2013 > Committer:Ethan J. Jackson Thu Dec 19 > 13:27:23 > 2013 > > ofproto: Handle flow installation and eviction in upcall. > > This patch moves flow installation and eviction from ofproto-dpif and > the main thread, into ofproto-dpif-upcall. This performs > significantly better (approximately 2x TCP_CRR improvement), and > allows ovs-vswitchd to maintain significantly larger datapath flow > tables. On top of that, it significantly simplifies the code, > retiring "struct facet" and friends. > > Signed-off-by: Ethan Jackson > Acked-by: Ben Pfaff Thanks Ben for looking into this issue. Should I send a formal patch for review by removing this check (n_flows > 2000) ? Warm Regards, Vishal Ajmera ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
Re: [ovs-discuss] ofproto-dpif-upcall: reg. udpif_revalidator thread
On Thu, Jul 05, 2018 at 12:02:31PM +, Vishal Deep Ajmera wrote:
> Hi,
>
>
>
> The udpif_revalidator function uses dump-duration time to determine the flow
> limit and tunes it as per system conditions for e.g. if duration is more,
> flow limit is reduced and vice-versa. However I notice one issue with the
> logic used in the implementation:
>
>
>
> if (duration > 2000) {
>
> flow_limit /= duration / 1000;
>
> } else if (duration > 1300) {
>
> flow_limit = flow_limit * 3 / 4;
>
> } else if (duration < 1000 && n_flows > 2000
>
>&& flow_limit < n_flows * 1000 / duration) {
>
> flow_limit += 1000;
>
> }
>
>
>
> The issue is with small number of flows (n_flows < 2000). When revalidator
> does not get sufficient cpu cycles to run (duration > 2000) and the
> flow_limit is reduced to < 2000. Now, even if revalidator gets sufficient
> cycles from cpu (duration < 1000) the flow_limit will not increase since
> n_flows is still small. Also, it will not allow for addition of new flows due
> to low flow_limit. This result in ovs stuck with low flow-limit and no
> automatic recovery.
>
>
>
> In my opinion the check n_flows > 2000 is incorrect in above code. Is my
> understanding correct or am I missing something ?
At first glance it looks like you're correct, but this code was added in
2013 with the following commit and hasn't changed since, so I wonder
whether we're missing something important. Ethan, you wrote this code,
do you have any thoughts?
commit e79a6c833e0d72370951d6f8841098103cbb0b2d
Author: Ethan J. Jackson Tue Sep 24 13:39:56
2013
Committer: Ethan J. Jackson Thu Dec 19 13:27:23
2013
ofproto: Handle flow installation and eviction in upcall.
This patch moves flow installation and eviction from ofproto-dpif and
the main thread, into ofproto-dpif-upcall. This performs
significantly better (approximately 2x TCP_CRR improvement), and
allows ovs-vswitchd to maintain significantly larger datapath flow
tables. On top of that, it significantly simplifies the code,
retiring "struct facet" and friends.
Signed-off-by: Ethan Jackson
Acked-by: Ben Pfaff
___
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
[ovs-discuss] ofproto-dpif-upcall: reg. udpif_revalidator thread
Hi,
The udpif_revalidator function uses dump-duration time to determine the flow
limit and tunes it as per system conditions for e.g. if duration is more, flow
limit is reduced and vice-versa. However I notice one issue with the logic used
in the implementation:
if (duration > 2000) {
flow_limit /= duration / 1000;
} else if (duration > 1300) {
flow_limit = flow_limit * 3 / 4;
} else if (duration < 1000 && n_flows > 2000
&& flow_limit < n_flows * 1000 / duration) {
flow_limit += 1000;
}
The issue is with small number of flows (n_flows < 2000). When revalidator does
not get sufficient cpu cycles to run (duration > 2000) and the flow_limit is
reduced to < 2000. Now, even if revalidator gets sufficient cycles from cpu
(duration < 1000) the flow_limit will not increase since n_flows is still
small. Also, it will not allow for addition of new flows due to low flow_limit.
This result in ovs stuck with low flow-limit and no automatic recovery.
In my opinion the check n_flows > 2000 is incorrect in above code. Is my
understanding correct or am I missing something ?
Warm Regards,
Vishal Ajmera
___
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
