On 9/4/25 6:28 PM, Eli Britstein wrote: > Levels are treated as severity ones. Default level remains INFO and two > additional levels WARN and ERR are added. > > Signed-off-by: Eli Britstein <[email protected]> > --- > lib/conntrack-tcp.c | 4 ++-- > lib/conntrack.c | 10 +++++----- > lib/coverage.c | 12 ++++++++++++ > lib/coverage.h | 6 ++++++ > lib/dpif-netdev.c | 24 ++++++++++++------------ > lib/dpif.c | 6 +++--- > lib/flow.c | 8 ++++---- > lib/hindex.c | 2 +- > lib/ipf.c | 6 +++--- > lib/lockfile.c | 2 +- > lib/netdev-afxdp.c | 8 ++++---- > lib/netdev-dpdk.c | 2 +- > lib/netdev-linux.c | 4 ++-- > lib/netdev-native-tnl.c | 4 ++-- > lib/netdev.c | 4 ++-- > lib/netlink-socket.c | 2 +- > lib/odp-execute.c | 34 +++++++++++++++++----------------- > lib/ovsdb-idl.c | 2 +- > lib/tc.c | 2 +- > ofproto/ofproto-dpif-upcall.c | 22 +++++++++++----------- > ofproto/ofproto-dpif-xlate.c | 4 ++-- > ofproto/ofproto-dpif.c | 2 +- > 22 files changed, 94 insertions(+), 76 deletions(-) > > diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c > index 8a7c98cc4..c3d52d231 100644 > --- a/lib/conntrack-tcp.c > +++ b/lib/conntrack-tcp.c > @@ -46,8 +46,8 @@ > #include "util.h" > > COVERAGE_DEFINE(conntrack_tcp_seq_chk_bypass); > -COVERAGE_DEFINE(conntrack_tcp_seq_chk_failed); > -COVERAGE_DEFINE(conntrack_invalid_tcp_flags); > +COVERAGE_DEFINE_ERR(conntrack_tcp_seq_chk_failed); > +COVERAGE_DEFINE_ERR(conntrack_invalid_tcp_flags);
Hi, Eli. Thanks for the patches! That's an interesting idea to highlight that some counters are more important than the others. However, since you're tying this to the log levels, they need to follow the same semantics as the log levels. In the log, error level is for internal errors inside the application that should never happen and must be fixed if they ever do. Warnings are for conditions that are important to note for the user to take some action, but they are normal and expected situations from the code perspective. By that definition, there should never be any coverage counters at the error level, as we shouldn't be counting internal errors via coverage mechanism. For example, the two counters above are just indicating some broken packets received by OVS, they are not OVS errors and should not be logged as errors. May be a warning, but we should be careful even with that, as there could be some stray packets even in the normal network that would hit one of these conditions and there is no point in warning users about that, unless the rate of these events is high. Some more thoughts on logging: some of the counters are self-explanatory, but, in general, they are not documented anywhere and should not really be, as they are internal, can change meaning over time, and only make sense for a particular version of the code. So, only someone who knows the code, can actually understand them, not the average user. Throwing warnings about something users can't understand does not sound like a good idea as it will just increase the volume of questions from confused users. We should be very careful at which counters to warn about and at what rate. For the implementation, I think, in many cases it's important to print all the counters together to understand the full picture of what is going on in the system. This change splits counters into groups and prints them separately, breaking the sorting order and potentially printing one part, but not another as hashes are compared separately, which may be harder to navigate. It may also be a little confusing if one counter doesn't change while some others do, but we keep warning about the old value on every dump. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
