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? //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
