On Mon, Jan 16, 2023 at 7:14 AM Eelco Chaudron <echau...@redhat.com> wrote: > > > > On 18 Oct 2022, at 18:08, Adrian Moreno wrote: > > > On 10/14/22 19:49, Han Zhou wrote: > >> > >> > >> On Thu, Oct 13, 2022 at 11:43 PM Adrian Moreno <amore...@redhat.com <mailto:amore...@redhat.com>> wrote: > >>> > >>> Hi Han. > >>> > >>> On 8/9/22 03:40, Han Zhou wrote: > >>>> Today the minimum value for this setting is 1. This patch allows it to > >>>> be 0, meaning not checking pps at all, and always do revalidation. > >>>> > >>>> This is particularly useful for environments where some of the > >>>> applications with long-lived connections may have very low traffic for > >>>> certain period but have high rate of burst periodically. It is desirable > >>>> to keep the datapath flows instead of periodically deleting them to > >>>> avoid burst of packet miss to userspace. > >>>> > >>>> When setting to 0, there may be more datapath flows to be revalidated, > >>>> resulting in higher CPU cost of revalidator threads. This is the > >>>> downside but in certain cases this is still more desirable than packet > >>>> misses to user space. > >>>> > >>> I am trying to quantify this CPU cost. Do you have any numbers so we understand > >>> the effects of disabling pps-based evictions? > >>> > >> Hi Adrian, > >> > >> Thanks for reviewing. First of all, the CPU cost is added to revalidator threads only, but saving cost for the handler threads which is more critical to the packet processing. > >> Now regarding the actual CPU cost of revalidator threads, the extra cost really depends on the traffic pattern - how many long lived DP flows are there with pps smaller than <min-revalidator-pps>. The more such DP flows, the more revalidtor CPU, of course. We don't see a problem when there are 10k DP flows when setting min-revalidator-pps to 0, and this is on a DPU with small number of less powerful ARM cores. It also depends on whether it is a dedicated network node/DPU where it is ok for OVS to take a significant portion of the CPU or it is a compute node where more cores are supposed to be reserved for applications. > >> In any case, this is just an option and totally depends on user settings according to their use case, as explained in the commit message. > >> > > > > I agree. There are many factors that come into play. My concern is that we have so many knobs that tweak the revalidator/handler load distribution that depend on so many external factors that it's quite complicated to determine when to recommend their use. > > > > That's why I wanted to have some ballpark numbers to get an intuition of the potential side effects. > > > > Having said that, I think this particular proposal is beneficial as I also have seen drops caused by long-lived connections. > > I do not see anything wrong with adding this feature, however, the documentation needs to be clear on what 0 means, i.e. does it result in always revalidate or never revalidate?
Thanks Eelco. I thought that min-revalidate-pps=0 naturally means revalidating all flows, because pps > 0 would include all flows. However, probably the "disable" in my commit title was confusing, and some may think it was trying to disable the revalidation (while I meant disable the option). So, I updated the title and also updated the document to avoid confusion in v2. PTAL: https://patchwork.ozlabs.org/project/openvswitch/patch/20230117030129.1447363-1-hz...@ovn.org/ Thanks, Han > > > //Eelco > > > >>>> Signed-off-by: Han Zhou <hz...@ovn.org <mailto:hz...@ovn.org>> > >>>> --- > >>>> ofproto/ofproto-dpif-upcall.c | 4 ++++ > >>>> ofproto/ofproto.c | 2 +- > >>>> vswitchd/vswitch.xml | 2 +- > >>>> 3 files changed, 6 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > >>>> index 57f94df54..23b52ef40 100644 > >>>> --- a/ofproto/ofproto-dpif-upcall.c > >>>> +++ b/ofproto/ofproto-dpif-upcall.c > >>>> @@ -2079,6 +2079,10 @@ should_revalidate(const struct udpif *udpif, uint64_t packets, > >>>> { > >>>> long long int metric, now, duration; > >>>> > >>>> + if (!ofproto_min_revalidate_pps) { > >>>> + return true; > >>>> + } > >>>> + > >>>> if (!used) { > >>>> /* Always revalidate the first time a flow is dumped. */ > >>>> return true; > >>>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > >>>> index 3a527683c..259fad4b5 100644 > >>>> --- a/ofproto/ofproto.c > >>>> +++ b/ofproto/ofproto.c > >>>> @@ -723,7 +723,7 @@ ofproto_set_max_revalidator(unsigned max_revalidator) > >>>> void > >>>> ofproto_set_min_revalidate_pps(unsigned min_revalidate_pps) > >>>> { > >>>> - ofproto_min_revalidate_pps = min_revalidate_pps ? min_revalidate_pps : 1; > >>>> + ofproto_min_revalidate_pps = min_revalidate_pps; > >>>> } > >>>> > >>>> /* If forward_bpdu is true, the NORMAL action will forward frames with > >>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > >>>> index 4a9284f6b..2222c1296 100644 > >>>> --- a/vswitchd/vswitch.xml > >>>> +++ b/vswitchd/vswitch.xml > >>>> @@ -212,7 +212,7 @@ > >>>> </column> > >>>> > >>>> <column name="other_config" key="min-revalidate-pps" > >>>> - type='{"type": "integer", "minInteger": 1}'> > >>>> + type='{"type": "integer", "minInteger": 0}'> > >>>> <p> > >>>> Set minimum pps that flow must have in order to be revalidated when > >>>> revalidation duration exceeds half of max-revalidator config variable. > >>> > >>> -- > >>> Adrián Moreno > >>> > > > > -- > > Adrián Moreno > > > > _______________________________________________ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
