| Message ID | 161972525935.137484.11611813153589197465.stgit@fed.void |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] conntrack: add coverage counters for L3 bad checksum | expand |
On 29 Apr 2021, at 21:40, Paolo Valerio wrote: > similarly to what we already have for L4, add conntrack_l3csum_err > for the received packets with L3 bad checksum. > > Although, it basically covers IPv4, let's keep the name generic. > > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- > lib/conntrack.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 99198a601..70cdcc12a 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -45,6 +45,7 @@ VLOG_DEFINE_THIS_MODULE(conntrack); > > COVERAGE_DEFINE(conntrack_full); > COVERAGE_DEFINE(conntrack_long_cleanup); > +COVERAGE_DEFINE(conntrack_l3csum_err); > COVERAGE_DEFINE(conntrack_l4csum_err); > > struct conn_lookup_ctx { > @@ -1613,6 +1614,7 @@ extract_l3_ipv4(struct conn_key *key, const void > *data, size_t size, > } > > if (validate_checksum && csum(data, ip_len) != 0) { > + COVERAGE_INC(conntrack_l3csum_err); > return false; > } > > @@ -2051,6 +2053,7 @@ conn_key_extract(struct conntrack *ct, struct > dp_packet *pkt, ovs_be16 dl_type, > bool hwol_bad_l3_csum = dp_packet_ip_checksum_bad(pkt); > if (hwol_bad_l3_csum) { > ok = false; > + COVERAGE_INC(conntrack_l3csum_err); I think this needs to go outside the if() statement, as the checksum can also fail below if HW support is not enabled. > } else { > bool hwol_good_l3_csum = dp_packet_ip_checksum_valid(pkt) > || dp_packet_hwol_is_ipv4(pkt); } if (!ok) { COVERAGE_INC(conntrack_l3csum_err); } > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 30 Apr 2021, at 9:25, Eelco Chaudron wrote: > On 29 Apr 2021, at 21:40, Paolo Valerio wrote: > >> similarly to what we already have for L4, add conntrack_l3csum_err >> for the received packets with L3 bad checksum. >> >> Although, it basically covers IPv4, let's keep the name generic. >> >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> Ignore my previous email, as I missed some code due to the partial diff :( Acked-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> lib/conntrack.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 99198a601..70cdcc12a 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -45,6 +45,7 @@ VLOG_DEFINE_THIS_MODULE(conntrack); >> >> COVERAGE_DEFINE(conntrack_full); >> COVERAGE_DEFINE(conntrack_long_cleanup); >> +COVERAGE_DEFINE(conntrack_l3csum_err); >> COVERAGE_DEFINE(conntrack_l4csum_err); >> >> struct conn_lookup_ctx { >> @@ -1613,6 +1614,7 @@ extract_l3_ipv4(struct conn_key *key, const >> void *data, size_t size, >> } >> >> if (validate_checksum && csum(data, ip_len) != 0) { >> + COVERAGE_INC(conntrack_l3csum_err); >> return false; >> } >> >> @@ -2051,6 +2053,7 @@ conn_key_extract(struct conntrack *ct, struct >> dp_packet *pkt, ovs_be16 dl_type, >> bool hwol_bad_l3_csum = dp_packet_ip_checksum_bad(pkt); >> if (hwol_bad_l3_csum) { >> ok = false; >> + COVERAGE_INC(conntrack_l3csum_err); > > I think this needs to go outside the if() statement, as the checksum > can also fail below if HW support is not enabled. > >> } else { >> bool hwol_good_l3_csum = >> dp_packet_ip_checksum_valid(pkt) >> || dp_packet_hwol_is_ipv4(pkt); > > } > if (!ok) { > COVERAGE_INC(conntrack_l3csum_err); > } > > >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
"Eelco Chaudron" <echaudro@redhat.com> writes: > On 30 Apr 2021, at 9:25, Eelco Chaudron wrote: > >> On 29 Apr 2021, at 21:40, Paolo Valerio wrote: >> >>> similarly to what we already have for L4, add conntrack_l3csum_err >>> for the received packets with L3 bad checksum. >>> >>> Although, it basically covers IPv4, let's keep the name generic. >>> >>> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > > Ignore my previous email, as I missed some code due to the partial diff > :( > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > Thank you Eelco. well, some are really missing. The patch doesn't tell the whole story, fragments are not included. Need to respin adding one for them. Sorry for the noise. Paolo
diff --git a/lib/conntrack.c b/lib/conntrack.c index 99198a601..70cdcc12a 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -45,6 +45,7 @@ VLOG_DEFINE_THIS_MODULE(conntrack); COVERAGE_DEFINE(conntrack_full); COVERAGE_DEFINE(conntrack_long_cleanup); +COVERAGE_DEFINE(conntrack_l3csum_err); COVERAGE_DEFINE(conntrack_l4csum_err); struct conn_lookup_ctx { @@ -1613,6 +1614,7 @@ extract_l3_ipv4(struct conn_key *key, const void *data, size_t size, } if (validate_checksum && csum(data, ip_len) != 0) { + COVERAGE_INC(conntrack_l3csum_err); return false; } @@ -2051,6 +2053,7 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type, bool hwol_bad_l3_csum = dp_packet_ip_checksum_bad(pkt); if (hwol_bad_l3_csum) { ok = false; + COVERAGE_INC(conntrack_l3csum_err); } else { bool hwol_good_l3_csum = dp_packet_ip_checksum_valid(pkt) || dp_packet_hwol_is_ipv4(pkt);
similarly to what we already have for L4, add conntrack_l3csum_err for the received packets with L3 bad checksum. Although, it basically covers IPv4, let's keep the name generic. Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- lib/conntrack.c | 3 +++ 1 file changed, 3 insertions(+)