| Message ID | 161515948046.159683.4471336732483153418.stgit@fed.void |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] conntrack: increment coverage counter for all bad checksum cases | expand |
On Mon, Mar 8, 2021 at 7:24 AM Paolo Valerio <pvalerio@redhat.com> wrote: > > conntrack_l4csum_err gets incremented only when corrupted icmp > pass through conntrack. > Increase it for the remaining bad checksum cases including when > checksum is offloaded. This patch looks good to me, and one question, do you have plan to support l3 csum counters, such as "conntrack_l3csum_err". Reviewed-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- > lib/conntrack.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 99198a601..30277f6fd 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -1669,15 +1669,22 @@ static inline bool > checksum_valid(const struct conn_key *key, const void *data, size_t size, > const void *l3) > { > + bool valid; > + > if (key->dl_type == htons(ETH_TYPE_IP)) { > uint32_t csum = packet_csum_pseudoheader(l3); > - return csum_finish(csum_continue(csum, data, size)) == 0; > + valid = (csum_finish(csum_continue(csum, data, size)) == 0); > } else if (key->dl_type == htons(ETH_TYPE_IPV6)) { > - return packet_csum_upperlayer6(l3, data, key->nw_proto, size) == 0; > + valid = (packet_csum_upperlayer6(l3, data, key->nw_proto, size) == 0); > } else { > + valid = false; > + } > + > + if (!valid) { > COVERAGE_INC(conntrack_l4csum_err); > - return false; > } > + > + return valid; > } > > static inline bool > @@ -2076,6 +2083,8 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type, > ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); > return true; > } > + } else { > + COVERAGE_INC(conntrack_l4csum_err); > } > } > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Tonghao, Tonghao Zhang <xiangxia.m.yue@gmail.com> writes: > On Mon, Mar 8, 2021 at 7:24 AM Paolo Valerio <pvalerio@redhat.com> wrote: >> >> conntrack_l4csum_err gets incremented only when corrupted icmp >> pass through conntrack. >> Increase it for the remaining bad checksum cases including when >> checksum is offloaded. > This patch looks good to me, and one question, do you have plan to > support l3 csum counters, such as "conntrack_l3csum_err". > Reviewed-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > thanks for the review. I noticed this behaviour for the l4 counter while working on another patch and I fixed it, so there was no actual plan for l3, but I can add it in another patch if we think it's the case. Paolo >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >> --- >> lib/conntrack.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 99198a601..30277f6fd 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -1669,15 +1669,22 @@ static inline bool >> checksum_valid(const struct conn_key *key, const void *data, size_t size, >> const void *l3) >> { >> + bool valid; >> + >> if (key->dl_type == htons(ETH_TYPE_IP)) { >> uint32_t csum = packet_csum_pseudoheader(l3); >> - return csum_finish(csum_continue(csum, data, size)) == 0; >> + valid = (csum_finish(csum_continue(csum, data, size)) == 0); >> } else if (key->dl_type == htons(ETH_TYPE_IPV6)) { >> - return packet_csum_upperlayer6(l3, data, key->nw_proto, size) == 0; >> + valid = (packet_csum_upperlayer6(l3, data, key->nw_proto, size) == 0); >> } else { >> + valid = false; >> + } >> + >> + if (!valid) { >> COVERAGE_INC(conntrack_l4csum_err); >> - return false; >> } >> + >> + return valid; >> } >> >> static inline bool >> @@ -2076,6 +2083,8 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type, >> ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); >> return true; >> } >> + } else { >> + COVERAGE_INC(conntrack_l4csum_err); >> } >> } >> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > -- > Best regards, Tonghao
Paolo Valerio <pvalerio@redhat.com> writes: > conntrack_l4csum_err gets incremented only when corrupted icmp > pass through conntrack. > Increase it for the remaining bad checksum cases including when > checksum is offloaded. > > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- Missed the Fixes tag: Fixes: 38c69ccf8e29 ("conntrack: Add coverage count for l4csum error.")
On Wed, Jun 23, 2021 at 1:01 AM Paolo Valerio <pvalerio@redhat.com> wrote: > > Paolo Valerio <pvalerio@redhat.com> writes: > > > conntrack_l4csum_err gets incremented only when corrupted icmp > > pass through conntrack. > > Increase it for the remaining bad checksum cases including when > > checksum is offloaded. > > > > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > > --- > > Missed the Fixes tag: > > Fixes: 38c69ccf8e29 ("conntrack: Add coverage count for l4csum error.") Acked-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> Thanks! > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 6/23/21 3:44 PM, Tonghao Zhang wrote: > On Wed, Jun 23, 2021 at 1:01 AM Paolo Valerio <pvalerio@redhat.com> wrote: >> >> Paolo Valerio <pvalerio@redhat.com> writes: >> >>> conntrack_l4csum_err gets incremented only when corrupted icmp >>> pass through conntrack. >>> Increase it for the remaining bad checksum cases including when >>> checksum is offloaded. >>> >>> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >>> --- >> >> Missed the Fixes tag: >> >> Fixes: 38c69ccf8e29 ("conntrack: Add coverage count for l4csum error.") > Acked-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> Thanks! Applied. Backported down to 2.14. Best regards, Ilya Maximets.
diff --git a/lib/conntrack.c b/lib/conntrack.c index 99198a601..30277f6fd 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1669,15 +1669,22 @@ static inline bool checksum_valid(const struct conn_key *key, const void *data, size_t size, const void *l3) { + bool valid; + if (key->dl_type == htons(ETH_TYPE_IP)) { uint32_t csum = packet_csum_pseudoheader(l3); - return csum_finish(csum_continue(csum, data, size)) == 0; + valid = (csum_finish(csum_continue(csum, data, size)) == 0); } else if (key->dl_type == htons(ETH_TYPE_IPV6)) { - return packet_csum_upperlayer6(l3, data, key->nw_proto, size) == 0; + valid = (packet_csum_upperlayer6(l3, data, key->nw_proto, size) == 0); } else { + valid = false; + } + + if (!valid) { COVERAGE_INC(conntrack_l4csum_err); - return false; } + + return valid; } static inline bool @@ -2076,6 +2083,8 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type, ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); return true; } + } else { + COVERAGE_INC(conntrack_l4csum_err); } }
conntrack_l4csum_err gets incremented only when corrupted icmp pass through conntrack. Increase it for the remaining bad checksum cases including when checksum is offloaded. Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- lib/conntrack.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)