| Message ID | 20210517140434.59555-11-cian.ferriter@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | DPIF Framework + Optimizations | expand |
> From: Harry van Haaren <harry.van.haaren@intel.com> > > This commit refactors the function names from netdev_* > namespace to the dpcls_* namespace, as they are only used > by dpcls code. With the name change, it becomes more obvious > that the functions belong to dpcls functionality, and in the > dpif-netdev-private-dpcls.h header file. Thanks for the patch Cian/Harry. My one query would be could this not be rolled into one of the earlier refactor patches rather than a separate commit as it's straight forward IMO. Thanks Ian > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > --- > lib/dpif-netdev-private-dpcls.h | 6 ++---- > lib/dpif-netdev.c | 21 ++++++++++----------- > 2 files changed, 12 insertions(+), 15 deletions(-) > > diff --git a/lib/dpif-netdev-private-dpcls.h b/lib/dpif-netdev-private-dpcls.h > index f223a93e4..28c6a10ff 100644 > --- a/lib/dpif-netdev-private-dpcls.h > +++ b/lib/dpif-netdev-private-dpcls.h > @@ -97,10 +97,8 @@ struct dpcls_subtable { > > /* Generates a mask for each bit set in the subtable's miniflow. */ > void > -netdev_flow_key_gen_masks(const struct netdev_flow_key *tbl, > - uint64_t *mf_masks, > - const uint32_t mf_bits_u0, > - const uint32_t mf_bits_u1); > +dpcls_flow_key_gen_masks(const struct netdev_flow_key *tbl, uint64_t > *mf_masks, > + const uint32_t mf_bits_u0, const uint32_t mf_bits_u1); > > /* Matches a dpcls rule against the incoming packet in 'target' */ > bool dpcls_rule_matches_key(const struct dpcls_rule *rule, > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index f161ae7f5..04a4f71cb 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -8314,7 +8314,7 @@ dpcls_create_subtable(struct dpcls *cls, const > struct netdev_flow_key *mask) > subtable->mf_bits_set_unit0 = unit0; > subtable->mf_bits_set_unit1 = unit1; > subtable->mf_masks = xmalloc(sizeof(uint64_t) * (unit0 + unit1)); > - netdev_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1); > + dpcls_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1); > > /* Get the preferred subtable search function for this (u0,u1) subtable. > * The function is guaranteed to always return a valid implementation, and > @@ -8489,11 +8489,10 @@ dpcls_remove(struct dpcls *cls, struct dpcls_rule > *rule) > } > } > > -/* Inner loop for mask generation of a unit, see > netdev_flow_key_gen_masks. */ > +/* Inner loop for mask generation of a unit, see > dpcls_flow_key_gen_masks. */ > static inline void > -netdev_flow_key_gen_mask_unit(uint64_t iter, > - const uint64_t count, > - uint64_t *mf_masks) > +dpcls_flow_key_gen_mask_unit(uint64_t iter, const uint64_t count, > + uint64_t *mf_masks) > { > int i; > for (i = 0; i < count; i++) { > @@ -8514,16 +8513,16 @@ netdev_flow_key_gen_mask_unit(uint64_t iter, > * @param mf_bits_unit0 Number of bits set in unit0 of the miniflow > */ > void > -netdev_flow_key_gen_masks(const struct netdev_flow_key *tbl, > - uint64_t *mf_masks, > - const uint32_t mf_bits_u0, > - const uint32_t mf_bits_u1) > +dpcls_flow_key_gen_masks(const struct netdev_flow_key *tbl, > + uint64_t *mf_masks, > + const uint32_t mf_bits_u0, > + const uint32_t mf_bits_u1) > { > uint64_t iter_u0 = tbl->mf.map.bits[0]; > uint64_t iter_u1 = tbl->mf.map.bits[1]; > > - netdev_flow_key_gen_mask_unit(iter_u0, mf_bits_u0, &mf_masks[0]); > - netdev_flow_key_gen_mask_unit(iter_u1, mf_bits_u1, > &mf_masks[mf_bits_u0]); > + dpcls_flow_key_gen_mask_unit(iter_u0, mf_bits_u0, &mf_masks[0]); > + dpcls_flow_key_gen_mask_unit(iter_u1, mf_bits_u1, > &mf_masks[mf_bits_u0]); > } > > /* Returns true if 'target' satisfies 'key' in 'mask', that is, if each 1-bit > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Ian, Thanks for the review. My responses are inline. > -----Original Message----- > From: Stokes, Ian <ian.stokes@intel.com> > Sent: Wednesday 2 June 2021 19:56 > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com> > Cc: i.maximets@ovn.org > Subject: RE: [ovs-dev] [v12 10/16] dpif-netdev/dpcls: Refactor function names to dpcls. > > > From: Harry van Haaren <harry.van.haaren@intel.com> > > > > This commit refactors the function names from netdev_* > > namespace to the dpcls_* namespace, as they are only used > > by dpcls code. With the name change, it becomes more obvious > > that the functions belong to dpcls functionality, and in the > > dpif-netdev-private-dpcls.h header file. > > Thanks for the patch Cian/Harry. > > My one query would be could this not be rolled into one of the earlier refactor patches rather than a separate commit as it's straight > forward IMO. > I'm happy to squash this into an earlier patch if you think it makes more sense. I'll squash it into the large refactor patch ([ovs-dev,v12,01/16] dpif-netdev: Refactor to multiple header files.) in the next version. > Thanks > Ian > > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- > > lib/dpif-netdev-private-dpcls.h | 6 ++---- > > lib/dpif-netdev.c | 21 ++++++++++----------- > > 2 files changed, 12 insertions(+), 15 deletions(-) > > > > diff --git a/lib/dpif-netdev-private-dpcls.h b/lib/dpif-netdev-private-dpcls.h > > index f223a93e4..28c6a10ff 100644 > > --- a/lib/dpif-netdev-private-dpcls.h > > +++ b/lib/dpif-netdev-private-dpcls.h > > @@ -97,10 +97,8 @@ struct dpcls_subtable { > > > > /* Generates a mask for each bit set in the subtable's miniflow. */ > > void > > -netdev_flow_key_gen_masks(const struct netdev_flow_key *tbl, > > - uint64_t *mf_masks, > > - const uint32_t mf_bits_u0, > > - const uint32_t mf_bits_u1); > > +dpcls_flow_key_gen_masks(const struct netdev_flow_key *tbl, uint64_t > > *mf_masks, > > + const uint32_t mf_bits_u0, const uint32_t mf_bits_u1); > > > > /* Matches a dpcls rule against the incoming packet in 'target' */ > > bool dpcls_rule_matches_key(const struct dpcls_rule *rule, > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index f161ae7f5..04a4f71cb 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -8314,7 +8314,7 @@ dpcls_create_subtable(struct dpcls *cls, const > > struct netdev_flow_key *mask) > > subtable->mf_bits_set_unit0 = unit0; > > subtable->mf_bits_set_unit1 = unit1; > > subtable->mf_masks = xmalloc(sizeof(uint64_t) * (unit0 + unit1)); > > - netdev_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1); > > + dpcls_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1); > > > > /* Get the preferred subtable search function for this (u0,u1) subtable. > > * The function is guaranteed to always return a valid implementation, and > > @@ -8489,11 +8489,10 @@ dpcls_remove(struct dpcls *cls, struct dpcls_rule > > *rule) > > } > > } > > > > -/* Inner loop for mask generation of a unit, see > > netdev_flow_key_gen_masks. */ > > +/* Inner loop for mask generation of a unit, see > > dpcls_flow_key_gen_masks. */ > > static inline void > > -netdev_flow_key_gen_mask_unit(uint64_t iter, > > - const uint64_t count, > > - uint64_t *mf_masks) > > +dpcls_flow_key_gen_mask_unit(uint64_t iter, const uint64_t count, > > + uint64_t *mf_masks) > > { > > int i; > > for (i = 0; i < count; i++) { > > @@ -8514,16 +8513,16 @@ netdev_flow_key_gen_mask_unit(uint64_t iter, > > * @param mf_bits_unit0 Number of bits set in unit0 of the miniflow > > */ > > void > > -netdev_flow_key_gen_masks(const struct netdev_flow_key *tbl, > > - uint64_t *mf_masks, > > - const uint32_t mf_bits_u0, > > - const uint32_t mf_bits_u1) > > +dpcls_flow_key_gen_masks(const struct netdev_flow_key *tbl, > > + uint64_t *mf_masks, > > + const uint32_t mf_bits_u0, > > + const uint32_t mf_bits_u1) > > { > > uint64_t iter_u0 = tbl->mf.map.bits[0]; > > uint64_t iter_u1 = tbl->mf.map.bits[1]; > > > > - netdev_flow_key_gen_mask_unit(iter_u0, mf_bits_u0, &mf_masks[0]); > > - netdev_flow_key_gen_mask_unit(iter_u1, mf_bits_u1, > > &mf_masks[mf_bits_u0]); > > + dpcls_flow_key_gen_mask_unit(iter_u0, mf_bits_u0, &mf_masks[0]); > > + dpcls_flow_key_gen_mask_unit(iter_u1, mf_bits_u1, > > &mf_masks[mf_bits_u0]); > > } > > > > /* Returns true if 'target' satisfies 'key' in 'mask', that is, if each 1-bit > > -- > > 2.31.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> Hi Ian, > > Thanks for the review. My responses are inline. > > > -----Original Message----- > > From: Stokes, Ian <ian.stokes@intel.com> > > Sent: Wednesday 2 June 2021 19:56 > > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van > Haaren, Harry <harry.van.haaren@intel.com> > > Cc: i.maximets@ovn.org > > Subject: RE: [ovs-dev] [v12 10/16] dpif-netdev/dpcls: Refactor function names > to dpcls. > > > > > From: Harry van Haaren <harry.van.haaren@intel.com> > > > > > > This commit refactors the function names from netdev_* > > > namespace to the dpcls_* namespace, as they are only used > > > by dpcls code. With the name change, it becomes more obvious > > > that the functions belong to dpcls functionality, and in the > > > dpif-netdev-private-dpcls.h header file. > > > > Thanks for the patch Cian/Harry. > > > > My one query would be could this not be rolled into one of the earlier refactor > patches rather than a separate commit as it's straight > > forward IMO. > > Sure it's straight forward and once it's summarized in the commit message of the refactor it should be ok. Regards Ian > > I'm happy to squash this into an earlier patch if you think it makes more sense. > I'll squash it into the large refactor patch ([ovs-dev,v12,01/16] dpif-netdev: > Refactor to multiple header files.) in the next version. > > > Thanks > > Ian > > > > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > > --- > > > lib/dpif-netdev-private-dpcls.h | 6 ++---- > > > lib/dpif-netdev.c | 21 ++++++++++----------- > > > 2 files changed, 12 insertions(+), 15 deletions(-) > > > > > > diff --git a/lib/dpif-netdev-private-dpcls.h b/lib/dpif-netdev-private-dpcls.h > > > index f223a93e4..28c6a10ff 100644 > > > --- a/lib/dpif-netdev-private-dpcls.h > > > +++ b/lib/dpif-netdev-private-dpcls.h > > > @@ -97,10 +97,8 @@ struct dpcls_subtable { > > > > > > /* Generates a mask for each bit set in the subtable's miniflow. */ > > > void > > > -netdev_flow_key_gen_masks(const struct netdev_flow_key *tbl, > > > - uint64_t *mf_masks, > > > - const uint32_t mf_bits_u0, > > > - const uint32_t mf_bits_u1); > > > +dpcls_flow_key_gen_masks(const struct netdev_flow_key *tbl, uint64_t > > > *mf_masks, > > > + const uint32_t mf_bits_u0, const uint32_t mf_bits_u1); > > > > > > /* Matches a dpcls rule against the incoming packet in 'target' */ > > > bool dpcls_rule_matches_key(const struct dpcls_rule *rule, > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > > index f161ae7f5..04a4f71cb 100644 > > > --- a/lib/dpif-netdev.c > > > +++ b/lib/dpif-netdev.c > > > @@ -8314,7 +8314,7 @@ dpcls_create_subtable(struct dpcls *cls, const > > > struct netdev_flow_key *mask) > > > subtable->mf_bits_set_unit0 = unit0; > > > subtable->mf_bits_set_unit1 = unit1; > > > subtable->mf_masks = xmalloc(sizeof(uint64_t) * (unit0 + unit1)); > > > - netdev_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1); > > > + dpcls_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1); > > > > > > /* Get the preferred subtable search function for this (u0,u1) subtable. > > > * The function is guaranteed to always return a valid implementation, and > > > @@ -8489,11 +8489,10 @@ dpcls_remove(struct dpcls *cls, struct > dpcls_rule > > > *rule) > > > } > > > } > > > > > > -/* Inner loop for mask generation of a unit, see > > > netdev_flow_key_gen_masks. */ > > > +/* Inner loop for mask generation of a unit, see > > > dpcls_flow_key_gen_masks. */ > > > static inline void > > > -netdev_flow_key_gen_mask_unit(uint64_t iter, > > > - const uint64_t count, > > > - uint64_t *mf_masks) > > > +dpcls_flow_key_gen_mask_unit(uint64_t iter, const uint64_t count, > > > + uint64_t *mf_masks) > > > { > > > int i; > > > for (i = 0; i < count; i++) { > > > @@ -8514,16 +8513,16 @@ netdev_flow_key_gen_mask_unit(uint64_t iter, > > > * @param mf_bits_unit0 Number of bits set in unit0 of the miniflow > > > */ > > > void > > > -netdev_flow_key_gen_masks(const struct netdev_flow_key *tbl, > > > - uint64_t *mf_masks, > > > - const uint32_t mf_bits_u0, > > > - const uint32_t mf_bits_u1) > > > +dpcls_flow_key_gen_masks(const struct netdev_flow_key *tbl, > > > + uint64_t *mf_masks, > > > + const uint32_t mf_bits_u0, > > > + const uint32_t mf_bits_u1) > > > { > > > uint64_t iter_u0 = tbl->mf.map.bits[0]; > > > uint64_t iter_u1 = tbl->mf.map.bits[1]; > > > > > > - netdev_flow_key_gen_mask_unit(iter_u0, mf_bits_u0, &mf_masks[0]); > > > - netdev_flow_key_gen_mask_unit(iter_u1, mf_bits_u1, > > > &mf_masks[mf_bits_u0]); > > > + dpcls_flow_key_gen_mask_unit(iter_u0, mf_bits_u0, &mf_masks[0]); > > > + dpcls_flow_key_gen_mask_unit(iter_u1, mf_bits_u1, > > > &mf_masks[mf_bits_u0]); > > > } > > > > > > /* Returns true if 'target' satisfies 'key' in 'mask', that is, if each 1-bit > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/dpif-netdev-private-dpcls.h b/lib/dpif-netdev-private-dpcls.h index f223a93e4..28c6a10ff 100644 --- a/lib/dpif-netdev-private-dpcls.h +++ b/lib/dpif-netdev-private-dpcls.h @@ -97,10 +97,8 @@ struct dpcls_subtable { /* Generates a mask for each bit set in the subtable's miniflow. */ void -netdev_flow_key_gen_masks(const struct netdev_flow_key *tbl, - uint64_t *mf_masks, - const uint32_t mf_bits_u0, - const uint32_t mf_bits_u1); +dpcls_flow_key_gen_masks(const struct netdev_flow_key *tbl, uint64_t *mf_masks, + const uint32_t mf_bits_u0, const uint32_t mf_bits_u1); /* Matches a dpcls rule against the incoming packet in 'target' */ bool dpcls_rule_matches_key(const struct dpcls_rule *rule, diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index f161ae7f5..04a4f71cb 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -8314,7 +8314,7 @@ dpcls_create_subtable(struct dpcls *cls, const struct netdev_flow_key *mask) subtable->mf_bits_set_unit0 = unit0; subtable->mf_bits_set_unit1 = unit1; subtable->mf_masks = xmalloc(sizeof(uint64_t) * (unit0 + unit1)); - netdev_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1); + dpcls_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1); /* Get the preferred subtable search function for this (u0,u1) subtable. * The function is guaranteed to always return a valid implementation, and @@ -8489,11 +8489,10 @@ dpcls_remove(struct dpcls *cls, struct dpcls_rule *rule) } } -/* Inner loop for mask generation of a unit, see netdev_flow_key_gen_masks. */ +/* Inner loop for mask generation of a unit, see dpcls_flow_key_gen_masks. */ static inline void -netdev_flow_key_gen_mask_unit(uint64_t iter, - const uint64_t count, - uint64_t *mf_masks) +dpcls_flow_key_gen_mask_unit(uint64_t iter, const uint64_t count, + uint64_t *mf_masks) { int i; for (i = 0; i < count; i++) { @@ -8514,16 +8513,16 @@ netdev_flow_key_gen_mask_unit(uint64_t iter, * @param mf_bits_unit0 Number of bits set in unit0 of the miniflow */ void -netdev_flow_key_gen_masks(const struct netdev_flow_key *tbl, - uint64_t *mf_masks, - const uint32_t mf_bits_u0, - const uint32_t mf_bits_u1) +dpcls_flow_key_gen_masks(const struct netdev_flow_key *tbl, + uint64_t *mf_masks, + const uint32_t mf_bits_u0, + const uint32_t mf_bits_u1) { uint64_t iter_u0 = tbl->mf.map.bits[0]; uint64_t iter_u1 = tbl->mf.map.bits[1]; - netdev_flow_key_gen_mask_unit(iter_u0, mf_bits_u0, &mf_masks[0]); - netdev_flow_key_gen_mask_unit(iter_u1, mf_bits_u1, &mf_masks[mf_bits_u0]); + dpcls_flow_key_gen_mask_unit(iter_u0, mf_bits_u0, &mf_masks[0]); + dpcls_flow_key_gen_mask_unit(iter_u1, mf_bits_u1, &mf_masks[mf_bits_u0]); } /* Returns true if 'target' satisfies 'key' in 'mask', that is, if each 1-bit