| Message ID | 20200929200746.541833-1-fbl@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] ofproto-dpif-upcall: Log the emergency flow flush. | expand |
Bleep bloop. Greetings Flavio Leitner, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 82 characters long (recommended limit is 79)
#60 FILE: ofproto/ofproto-dpif-upcall.c:2665:
PRIuSIZE"). Starting to delete flows unconditionally"
Lines checked: 71, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 29 Sep 2020, at 22:07, Flavio Leitner wrote: > When the number of flows in the datapath reaches twice the > maximum, revalidators will delete all flows as an emergency > action to recover. In that case, log a message with values > and increase a coverage counter. > > Signed-off-by: Flavio Leitner <fbl@redhat.com> > --- Other than the line being over 79 characters, see 0-day robot response, it looks fine to me. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On Wed, Sep 30, 2020 at 08:45:07AM +0200, Eelco Chaudron wrote: > > > On 29 Sep 2020, at 22:07, Flavio Leitner wrote: > > > When the number of flows in the datapath reaches twice the > > maximum, revalidators will delete all flows as an emergency > > action to recover. In that case, log a message with values > > and increase a coverage counter. > > > > Signed-off-by: Flavio Leitner <fbl@redhat.com> > > --- > > Other than the line being over 79 characters, see 0-day robot response, it > looks fine to me. I tried to break it in different ways but none looked better than what I posted to me. I am open to suggestions. Thanks for reviewing it! fbl > > Acked-by: Eelco Chaudron <echaudro@redhat.com> >
On 30 Sep 2020, at 13:19, Flavio Leitner wrote: > On Wed, Sep 30, 2020 at 08:45:07AM +0200, Eelco Chaudron wrote: >> >> >> On 29 Sep 2020, at 22:07, Flavio Leitner wrote: >> >>> When the number of flows in the datapath reaches twice the >>> maximum, revalidators will delete all flows as an emergency >>> action to recover. In that case, log a message with values >>> and increase a coverage counter. >>> >>> Signed-off-by: Flavio Leitner <fbl@redhat.com> >>> --- >> >> Other than the line being over 79 characters, see 0-day robot response, it >> looks fine to me. > > I tried to break it in different ways but none looked > better than what I posted to me. I am open to suggestions. Looking at it, it’s not getting better ;) I would say leave it as is! > Thanks for reviewing it! > fbl > >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> > > -- > fbl
On 9/30/20 1:24 PM, Eelco Chaudron wrote: > > > On 30 Sep 2020, at 13:19, Flavio Leitner wrote: > >> On Wed, Sep 30, 2020 at 08:45:07AM +0200, Eelco Chaudron wrote: >>> >>> >>> On 29 Sep 2020, at 22:07, Flavio Leitner wrote: >>> >>>> When the number of flows in the datapath reaches twice the >>>> maximum, revalidators will delete all flows as an emergency >>>> action to recover. In that case, log a message with values >>>> and increase a coverage counter. >>>> >>>> Signed-off-by: Flavio Leitner <fbl@redhat.com> >>>> --- >>> >>> Other than the line being over 79 characters, see 0-day robot response, it >>> looks fine to me. >> >> I tried to break it in different ways but none looked >> better than what I posted to me. I am open to suggestions. > > Looking at it, it’s not getting better ;) I would say leave it as is! I agree that it's hard to split this like in a good way while keeping the indentation on this level, but that is not strictly necessary. Following version looks fine, IMHO: VLOG_WARN_RL(&rlem, "Number of datapath flows (%"PRIuSIZE") twice as high as " "current dynamic flow limit (%"PRIuSIZE"). Starting to " "delete flows unconditionally as an emergency measure.", n_dp_flows, kill_all_limit); > >> Thanks for reviewing it! >> fbl >> >>> >>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>> >> >> -- >> fbl > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 9/30/20 2:16 PM, Ilya Maximets wrote: > On 9/30/20 1:24 PM, Eelco Chaudron wrote: >> >> >> On 30 Sep 2020, at 13:19, Flavio Leitner wrote: >> >>> On Wed, Sep 30, 2020 at 08:45:07AM +0200, Eelco Chaudron wrote: >>>> >>>> >>>> On 29 Sep 2020, at 22:07, Flavio Leitner wrote: >>>> >>>>> When the number of flows in the datapath reaches twice the >>>>> maximum, revalidators will delete all flows as an emergency >>>>> action to recover. In that case, log a message with values >>>>> and increase a coverage counter. >>>>> >>>>> Signed-off-by: Flavio Leitner <fbl@redhat.com> >>>>> --- >>>> >>>> Other than the line being over 79 characters, see 0-day robot response, it >>>> looks fine to me. >>> >>> I tried to break it in different ways but none looked >>> better than what I posted to me. I am open to suggestions. >> >> Looking at it, it’s not getting better ;) I would say leave it as is! > > I agree that it's hard to split this like in a good way while keeping the > indentation on this level, but that is not strictly necessary. > Following version looks fine, IMHO: > > VLOG_WARN_RL(&rlem, > "Number of datapath flows (%"PRIuSIZE") twice as high as " > "current dynamic flow limit (%"PRIuSIZE"). Starting to " > "delete flows unconditionally as an emergency measure.", > n_dp_flows, kill_all_limit); Maybe even this way (it splits the line in a more meaningful parts): VLOG_WARN_RL(&rlem, "Number of datapath flows (%"PRIuSIZE") twice as high as " "current dynamic flow limit (%"PRIuSIZE"). " "Starting to delete flows unconditionally " "as an emergency measure.", n_dp_flows, kill_all_limit); > >> >>> Thanks for reviewing it! >>> fbl >>> >>>> >>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>>> >>> >>> -- >>> fbl >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
On Wed, Sep 30, 2020 at 02:23:11PM +0200, Ilya Maximets wrote: > On 9/30/20 2:16 PM, Ilya Maximets wrote: > > On 9/30/20 1:24 PM, Eelco Chaudron wrote: > >> > >> > >> On 30 Sep 2020, at 13:19, Flavio Leitner wrote: > >> > >>> On Wed, Sep 30, 2020 at 08:45:07AM +0200, Eelco Chaudron wrote: > >>>> > >>>> > >>>> On 29 Sep 2020, at 22:07, Flavio Leitner wrote: > >>>> > >>>>> When the number of flows in the datapath reaches twice the > >>>>> maximum, revalidators will delete all flows as an emergency > >>>>> action to recover. In that case, log a message with values > >>>>> and increase a coverage counter. > >>>>> > >>>>> Signed-off-by: Flavio Leitner <fbl@redhat.com> > >>>>> --- > >>>> > >>>> Other than the line being over 79 characters, see 0-day robot response, it > >>>> looks fine to me. > >>> > >>> I tried to break it in different ways but none looked > >>> better than what I posted to me. I am open to suggestions. > >> > >> Looking at it, it’s not getting better ;) I would say leave it as is! > > > > I agree that it's hard to split this like in a good way while keeping the > > indentation on this level, but that is not strictly necessary. > > Following version looks fine, IMHO: > > > > VLOG_WARN_RL(&rlem, > > "Number of datapath flows (%"PRIuSIZE") twice as high as " > > "current dynamic flow limit (%"PRIuSIZE"). Starting to " > > "delete flows unconditionally as an emergency measure.", > > n_dp_flows, kill_all_limit); > > Maybe even this way (it splits the line in a more meaningful parts): > > VLOG_WARN_RL(&rlem, > "Number of datapath flows (%"PRIuSIZE") twice as high as " > "current dynamic flow limit (%"PRIuSIZE"). " > "Starting to delete flows unconditionally " > "as an emergency measure.", n_dp_flows, kill_all_limit); Sent v3: https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/375688.html Thanks,
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 195b01c13..35c240646 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -57,6 +57,7 @@ COVERAGE_DEFINE(upcall_ukey_contention); COVERAGE_DEFINE(upcall_ukey_replace); COVERAGE_DEFINE(revalidate_missed_dp_flow); COVERAGE_DEFINE(upcall_flow_limit_hit); +COVERAGE_DEFINE(upcall_flow_limit_kill); /* A thread that reads upcalls from dpif, forwards each upcall's packet, * and possibly sets up a kernel flow as a cache. */ @@ -2607,6 +2608,7 @@ revalidate(struct revalidator *revalidator) struct udpif *udpif = revalidator->udpif; struct dpif_flow_dump_thread *dump_thread; uint64_t dump_seq, reval_seq; + bool kill_warn_print = true; unsigned int flow_limit; dump_seq = seq_read(udpif->dump_seq); @@ -2623,6 +2625,7 @@ revalidate(struct revalidator *revalidator) long long int max_idle; long long int now; + size_t kill_all_limit; size_t n_dp_flows; bool kill_them_all; @@ -2650,7 +2653,23 @@ revalidate(struct revalidator *revalidator) COVERAGE_INC(upcall_flow_limit_hit); } - kill_them_all = n_dp_flows > flow_limit * 2; + kill_them_all = false; + kill_all_limit = flow_limit * 2; + if (OVS_UNLIKELY(n_dp_flows > kill_all_limit)) { + static struct vlog_rate_limit rlem = VLOG_RATE_LIMIT_INIT(1, 1); + + kill_them_all = true; + COVERAGE_INC(upcall_flow_limit_kill); + if (kill_warn_print) { + kill_warn_print = false; + VLOG_WARN_RL(&rlem, "Number of datapath flows (%"PRIuSIZE") " + "twice as high as current dynamic flow limit (%" + PRIuSIZE"). Starting to delete flows unconditionally" + " as an emergency measure.", n_dp_flows, + kill_all_limit); + } + } + max_idle = n_dp_flows > flow_limit ? 100 : ofproto_max_idle; udpif->dpif->current_ms = time_msec();
When the number of flows in the datapath reaches twice the maximum, revalidators will delete all flows as an emergency action to recover. In that case, log a message with values and increase a coverage counter. Signed-off-by: Flavio Leitner <fbl@redhat.com> --- ofproto/ofproto-dpif-upcall.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) v2: - updated the log message - print only once per cycle.