| Message ID | 20240307170810.63088-4-eric@garver.life |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dpif: probe support for OVS_ACTION_ATTR_DROP | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 3/7/24 18:08, Eric Garver wrote: > The next commit will convert a dp feature from bool to atomic_bool. As > such we have to add support to the macros and functions. We must pass by > reference instead of pass by value because all the atomic operations > require a reference. > > Signed-off-by: Eric Garver <eric@garver.life> > --- Not a full review, but a few quick comments inline. > ofproto/ofproto-dpif.c | 52 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 45 insertions(+), 7 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index d732198de5ea..4e22ee0e8045 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -717,6 +717,8 @@ close_dpif_backer(struct dpif_backer *backer, bool del) > } > > static void check_support(struct dpif_backer *backer); > +static void copy_support(struct dpif_backer_support *dst, > + struct dpif_backer_support *src); > > static int > open_dpif_backer(const char *type, struct dpif_backer **backerp) > @@ -837,7 +839,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp) > * 'boottime_support' can be checked to prevent 'support' to be changed > * beyond the datapath capabilities. In case 'support' is changed by > * the user, 'boottime_support' can be used to restore it. */ > - backer->bt_support = backer->rt_support; > + copy_support(&backer->bt_support, &backer->rt_support); > > return error; > } > @@ -1611,6 +1613,22 @@ CHECK_FEATURE__(ct_orig_tuple6, ct_orig_tuple6, ct_nw_proto, 1, ETH_TYPE_IPV6) > #undef CHECK_FEATURE > #undef CHECK_FEATURE__ > > +static void > +copy_support(struct dpif_backer_support *dst, struct dpif_backer_support *src) > +{ > +#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \ > + if (!strcmp(#TYPE, "atomic_bool")) { \ > + bool value; \ > + atomic_read_relaxed((atomic_bool *) &src->NAME, &value); \ > + atomic_store_relaxed((atomic_bool *) &dst->NAME, value); \ > + } else { \ > + dst->NAME = src->NAME; \ > + } > + > + DPIF_SUPPORT_FIELDS > +#undef DPIF_SUPPORT_FIELD We need to copy odp_support as well. > +} > + > static void > check_support(struct dpif_backer *backer) > { > @@ -6248,20 +6266,30 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED, > } > > static void > -show_dp_feature_bool(struct ds *ds, const char *feature, bool b) > +show_dp_feature_bool(struct ds *ds, const char *feature, const bool *b) > { > - ds_put_format(ds, "%s: %s\n", feature, b ? "Yes" : "No"); > + ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No"); > } > > static void > -show_dp_feature_size_t(struct ds *ds, const char *feature, size_t s) > +show_dp_feature_atomic_bool(struct ds *ds, const char *feature, > + const atomic_bool *b) > { > - ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s); > + bool value; > + atomic_read_relaxed((atomic_bool *) b, &value); > + ds_put_format(ds, "%s: %s\n", feature, value ? "Yes" : "No"); > +} > + > +static void > +show_dp_feature_size_t(struct ds *ds, const char *feature, const size_t *s) > +{ > + ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, *s); > } > > enum dpif_support_field_type { > DPIF_SUPPORT_FIELD_bool, > DPIF_SUPPORT_FIELD_size_t, > + DPIF_SUPPORT_FIELD_atomic_bool, > }; > > struct dpif_support_field { > @@ -6278,12 +6306,12 @@ static void > dpif_show_support(const struct dpif_backer_support *support, struct ds *ds) > { > #define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \ > - show_dp_feature_##TYPE (ds, TITLE, support->NAME); > + show_dp_feature_##TYPE (ds, TITLE, &support->NAME); > DPIF_SUPPORT_FIELDS > #undef DPIF_SUPPORT_FIELD > > #define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) \ > - show_dp_feature_##TYPE (ds, TITLE, support->odp.NAME ); > + show_dp_feature_##TYPE (ds, TITLE, &support->odp.NAME ); > ODP_SUPPORT_FIELDS > #undef ODP_SUPPORT_FIELD > } > @@ -6302,6 +6330,16 @@ display_support_field(const char *name, > b ? "true" : "false"); > break; > } > + case DPIF_SUPPORT_FIELD_atomic_bool: { > + bool v; > + bool b; Nit: better to define on a same line and have an empty line afterwards. > + atomic_read_relaxed((atomic_bool *) field->rt_ptr, &v); > + atomic_read_relaxed((atomic_bool *) field->bt_ptr, &b); > + ds_put_format(ds, "%s (%s) : [run time]:%s, [boot time]:%s\n", name, > + field->title, v ? "true" : "false", > + b ? "true" : "false"); > + break; > + } > case DPIF_SUPPORT_FIELD_size_t: > ds_put_format(ds, "%s (%s) : [run time]:%"PRIuSIZE > ", [boot time]:%"PRIuSIZE"\n", name,
On 3/7/24 20:18, Ilya Maximets wrote: > On 3/7/24 18:08, Eric Garver wrote: >> The next commit will convert a dp feature from bool to atomic_bool. As >> such we have to add support to the macros and functions. We must pass by >> reference instead of pass by value because all the atomic operations >> require a reference. >> >> Signed-off-by: Eric Garver <eric@garver.life> >> --- > > Not a full review, but a few quick comments inline. > >> ofproto/ofproto-dpif.c | 52 ++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 45 insertions(+), 7 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index d732198de5ea..4e22ee0e8045 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -717,6 +717,8 @@ close_dpif_backer(struct dpif_backer *backer, bool del) >> } >> >> static void check_support(struct dpif_backer *backer); >> +static void copy_support(struct dpif_backer_support *dst, >> + struct dpif_backer_support *src); >> >> static int >> open_dpif_backer(const char *type, struct dpif_backer **backerp) >> @@ -837,7 +839,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp) >> * 'boottime_support' can be checked to prevent 'support' to be changed >> * beyond the datapath capabilities. In case 'support' is changed by >> * the user, 'boottime_support' can be used to restore it. */ >> - backer->bt_support = backer->rt_support; >> + copy_support(&backer->bt_support, &backer->rt_support); >> >> return error; >> } >> @@ -1611,6 +1613,22 @@ CHECK_FEATURE__(ct_orig_tuple6, ct_orig_tuple6, ct_nw_proto, 1, ETH_TYPE_IPV6) >> #undef CHECK_FEATURE >> #undef CHECK_FEATURE__ >> >> +static void >> +copy_support(struct dpif_backer_support *dst, struct dpif_backer_support *src) >> +{ >> +#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \ >> + if (!strcmp(#TYPE, "atomic_bool")) { \ >> + bool value; \ >> + atomic_read_relaxed((atomic_bool *) &src->NAME, &value); \ >> + atomic_store_relaxed((atomic_bool *) &dst->NAME, value); \ >> + } else { \ >> + dst->NAME = src->NAME; \ >> + } >> + >> + DPIF_SUPPORT_FIELDS >> +#undef DPIF_SUPPORT_FIELD > > We need to copy odp_support as well. > >> +} >> + >> static void >> check_support(struct dpif_backer *backer) >> { >> @@ -6248,20 +6266,30 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED, >> } >> >> static void >> -show_dp_feature_bool(struct ds *ds, const char *feature, bool b) >> +show_dp_feature_bool(struct ds *ds, const char *feature, const bool *b) >> { >> - ds_put_format(ds, "%s: %s\n", feature, b ? "Yes" : "No"); >> + ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No"); >> } >> >> static void static void OVS_UNUSED Should silence the compiler errors. Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index d732198de5ea..4e22ee0e8045 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -717,6 +717,8 @@ close_dpif_backer(struct dpif_backer *backer, bool del) } static void check_support(struct dpif_backer *backer); +static void copy_support(struct dpif_backer_support *dst, + struct dpif_backer_support *src); static int open_dpif_backer(const char *type, struct dpif_backer **backerp) @@ -837,7 +839,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp) * 'boottime_support' can be checked to prevent 'support' to be changed * beyond the datapath capabilities. In case 'support' is changed by * the user, 'boottime_support' can be used to restore it. */ - backer->bt_support = backer->rt_support; + copy_support(&backer->bt_support, &backer->rt_support); return error; } @@ -1611,6 +1613,22 @@ CHECK_FEATURE__(ct_orig_tuple6, ct_orig_tuple6, ct_nw_proto, 1, ETH_TYPE_IPV6) #undef CHECK_FEATURE #undef CHECK_FEATURE__ +static void +copy_support(struct dpif_backer_support *dst, struct dpif_backer_support *src) +{ +#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \ + if (!strcmp(#TYPE, "atomic_bool")) { \ + bool value; \ + atomic_read_relaxed((atomic_bool *) &src->NAME, &value); \ + atomic_store_relaxed((atomic_bool *) &dst->NAME, value); \ + } else { \ + dst->NAME = src->NAME; \ + } + + DPIF_SUPPORT_FIELDS +#undef DPIF_SUPPORT_FIELD +} + static void check_support(struct dpif_backer *backer) { @@ -6248,20 +6266,30 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED, } static void -show_dp_feature_bool(struct ds *ds, const char *feature, bool b) +show_dp_feature_bool(struct ds *ds, const char *feature, const bool *b) { - ds_put_format(ds, "%s: %s\n", feature, b ? "Yes" : "No"); + ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No"); } static void -show_dp_feature_size_t(struct ds *ds, const char *feature, size_t s) +show_dp_feature_atomic_bool(struct ds *ds, const char *feature, + const atomic_bool *b) { - ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s); + bool value; + atomic_read_relaxed((atomic_bool *) b, &value); + ds_put_format(ds, "%s: %s\n", feature, value ? "Yes" : "No"); +} + +static void +show_dp_feature_size_t(struct ds *ds, const char *feature, const size_t *s) +{ + ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, *s); } enum dpif_support_field_type { DPIF_SUPPORT_FIELD_bool, DPIF_SUPPORT_FIELD_size_t, + DPIF_SUPPORT_FIELD_atomic_bool, }; struct dpif_support_field { @@ -6278,12 +6306,12 @@ static void dpif_show_support(const struct dpif_backer_support *support, struct ds *ds) { #define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \ - show_dp_feature_##TYPE (ds, TITLE, support->NAME); + show_dp_feature_##TYPE (ds, TITLE, &support->NAME); DPIF_SUPPORT_FIELDS #undef DPIF_SUPPORT_FIELD #define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) \ - show_dp_feature_##TYPE (ds, TITLE, support->odp.NAME ); + show_dp_feature_##TYPE (ds, TITLE, &support->odp.NAME ); ODP_SUPPORT_FIELDS #undef ODP_SUPPORT_FIELD } @@ -6302,6 +6330,16 @@ display_support_field(const char *name, b ? "true" : "false"); break; } + case DPIF_SUPPORT_FIELD_atomic_bool: { + bool v; + bool b; + atomic_read_relaxed((atomic_bool *) field->rt_ptr, &v); + atomic_read_relaxed((atomic_bool *) field->bt_ptr, &b); + ds_put_format(ds, "%s (%s) : [run time]:%s, [boot time]:%s\n", name, + field->title, v ? "true" : "false", + b ? "true" : "false"); + break; + } case DPIF_SUPPORT_FIELD_size_t: ds_put_format(ds, "%s (%s) : [run time]:%"PRIuSIZE ", [boot time]:%"PRIuSIZE"\n", name,
The next commit will convert a dp feature from bool to atomic_bool. As such we have to add support to the macros and functions. We must pass by reference instead of pass by value because all the atomic operations require a reference. Signed-off-by: Eric Garver <eric@garver.life> --- ofproto/ofproto-dpif.c | 52 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 7 deletions(-)