| Message ID | 20210430153129.27652-2-mark.d.gray@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dpif-netlink: Introduce per-cpu upcall dispatching | expand |
On Fri, Apr 30, 2021 at 11:31:27AM -0400, Mark Gray wrote: > 'n_handlers' and 'n_revalidators' are declared as type 'size_t'. > However, dpif_handlers_set() requires parameter 'n_handlers' as > type 'uint32_t'. This patch fixes this type mismatch. The change looks good, but I didn't understand the criteria used to do the change. For example, at udpif_stop_threads() you changed from 'size_t' to 'uint32_t', but variable 'i' is not required to be of the same type (marked in line below). However, I could find other similar cases left unchanged. fbl > > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- > ofproto/ofproto-dpif-upcall.c | 24 ++++++++++++------------ > ofproto/ofproto-dpif-upcall.h | 5 +++-- > ofproto/ofproto-provider.h | 2 +- > ofproto/ofproto.c | 2 +- > 4 files changed, 17 insertions(+), 16 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index ccf97266c0b9..88406fea1391 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -129,10 +129,10 @@ struct udpif { > struct dpif_backer *backer; /* Opaque dpif_backer pointer. */ > > struct handler *handlers; /* Upcall handlers. */ > - size_t n_handlers; > + uint32_t n_handlers; > > struct revalidator *revalidators; /* Flow revalidators. */ > - size_t n_revalidators; > + uint32_t n_revalidators; > > struct latch exit_latch; /* Tells child threads to exit. */ > > @@ -335,8 +335,8 @@ static int process_upcall(struct udpif *, struct upcall *, > struct ofpbuf *odp_actions, struct flow_wildcards *); > static void handle_upcalls(struct udpif *, struct upcall *, size_t n_upcalls); > static void udpif_stop_threads(struct udpif *, bool delete_flows); > -static void udpif_start_threads(struct udpif *, size_t n_handlers, > - size_t n_revalidators); > +static void udpif_start_threads(struct udpif *, uint32_t n_handlers, > + uint32_t n_revalidators); > static void udpif_pause_revalidators(struct udpif *); > static void udpif_resume_revalidators(struct udpif *); > static void *udpif_upcall_handler(void *); > @@ -522,7 +522,7 @@ static void > udpif_stop_threads(struct udpif *udpif, bool delete_flows) > { > if (udpif && (udpif->n_handlers != 0 || udpif->n_revalidators != 0)) { > - size_t i; > + uint32_t i; ^^^^^^^^^^^^^^^^^^^^^^^^ > > /* Tell the threads to exit. */ > latch_set(&udpif->exit_latch); > @@ -562,8 +562,8 @@ udpif_stop_threads(struct udpif *udpif, bool delete_flows) > > /* Starts the handler and revalidator threads. */ > static void > -udpif_start_threads(struct udpif *udpif, size_t n_handlers_, > - size_t n_revalidators_) > +udpif_start_threads(struct udpif *udpif, uint32_t n_handlers_, > + uint32_t n_revalidators_) > { > if (udpif && n_handlers_ && n_revalidators_) { > /* Creating a thread can take a significant amount of time on some > @@ -574,7 +574,7 @@ udpif_start_threads(struct udpif *udpif, size_t n_handlers_, > udpif->n_revalidators = n_revalidators_; > > udpif->handlers = xzalloc(udpif->n_handlers * sizeof *udpif->handlers); > - for (size_t i = 0; i < udpif->n_handlers; i++) { > + for (uint32_t i = 0; i < udpif->n_handlers; i++) { > struct handler *handler = &udpif->handlers[i]; > > handler->udpif = udpif; > @@ -632,8 +632,8 @@ udpif_resume_revalidators(struct udpif *udpif) > * datapath handle must have packet reception enabled before starting > * threads. */ > void > -udpif_set_threads(struct udpif *udpif, size_t n_handlers_, > - size_t n_revalidators_) > +udpif_set_threads(struct udpif *udpif, uint32_t n_handlers_, > + uint32_t n_revalidators_) > { > ovs_assert(udpif); > ovs_assert(n_handlers_ && n_revalidators_); > @@ -691,8 +691,8 @@ udpif_get_memory_usage(struct udpif *udpif, struct simap *usage) > void > udpif_flush(struct udpif *udpif) > { > - size_t n_handlers_ = udpif->n_handlers; > - size_t n_revalidators_ = udpif->n_revalidators; > + uint32_t n_handlers_ = udpif->n_handlers; > + uint32_t n_revalidators_ = udpif->n_revalidators; > > udpif_stop_threads(udpif, true); > dpif_flow_flush(udpif->dpif); > diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h > index 693107ae56c1..b4dfed32046e 100644 > --- a/ofproto/ofproto-dpif-upcall.h > +++ b/ofproto/ofproto-dpif-upcall.h > @@ -16,6 +16,7 @@ > #define OFPROTO_DPIF_UPCALL_H > > #include <stddef.h> > +#include <inttypes.h> > > struct dpif; > struct dpif_backer; > @@ -31,8 +32,8 @@ struct simap; > void udpif_init(void); > struct udpif *udpif_create(struct dpif_backer *, struct dpif *); > void udpif_run(struct udpif *udpif); > -void udpif_set_threads(struct udpif *, size_t n_handlers, > - size_t n_revalidators); > +void udpif_set_threads(struct udpif *, uint32_t n_handlers, > + uint32_t n_revalidators); > void udpif_destroy(struct udpif *); > void udpif_revalidate(struct udpif *); > void udpif_get_memory_usage(struct udpif *, struct simap *usage); > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 9ad2b71d23eb..57c7d17cb28f 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -534,7 +534,7 @@ extern unsigned ofproto_min_revalidate_pps; > > /* Number of upcall handler and revalidator threads. Only affects the > * ofproto-dpif implementation. */ > -extern size_t n_handlers, n_revalidators; > +extern uint32_t n_handlers, n_revalidators; > > static inline struct rule *rule_from_cls_rule(const struct cls_rule *); > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index b91517cd250d..69e5834e766c 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -309,7 +309,7 @@ unsigned ofproto_max_idle = OFPROTO_MAX_IDLE_DEFAULT; > unsigned ofproto_max_revalidator = OFPROTO_MAX_REVALIDATOR_DEFAULT; > unsigned ofproto_min_revalidate_pps = OFPROTO_MIN_REVALIDATE_PPS_DEFAULT; > > -size_t n_handlers, n_revalidators; > +uint32_t n_handlers, n_revalidators; > > /* Map from datapath name to struct ofproto, for use by unixctl commands. */ > static struct hmap all_ofprotos = HMAP_INITIALIZER(&all_ofprotos); > -- > 2.27.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 28/05/2021 20:50, Flavio Leitner wrote: > On Fri, Apr 30, 2021 at 11:31:27AM -0400, Mark Gray wrote: >> 'n_handlers' and 'n_revalidators' are declared as type 'size_t'. >> However, dpif_handlers_set() requires parameter 'n_handlers' as >> type 'uint32_t'. This patch fixes this type mismatch. > The change looks good, but I didn't understand the criteria used > to do the change. For example, at udpif_stop_threads() you changed > from 'size_t' to 'uint32_t', but variable 'i' is not required > to be of the same type (marked in line below). However, I could > find other similar cases left unchanged. > > fbl > Yes. The changes that you highlighted seem a bit arbitrary. I removed them and just left the updates to the various function signatures and variable declarations.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index ccf97266c0b9..88406fea1391 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -129,10 +129,10 @@ struct udpif { struct dpif_backer *backer; /* Opaque dpif_backer pointer. */ struct handler *handlers; /* Upcall handlers. */ - size_t n_handlers; + uint32_t n_handlers; struct revalidator *revalidators; /* Flow revalidators. */ - size_t n_revalidators; + uint32_t n_revalidators; struct latch exit_latch; /* Tells child threads to exit. */ @@ -335,8 +335,8 @@ static int process_upcall(struct udpif *, struct upcall *, struct ofpbuf *odp_actions, struct flow_wildcards *); static void handle_upcalls(struct udpif *, struct upcall *, size_t n_upcalls); static void udpif_stop_threads(struct udpif *, bool delete_flows); -static void udpif_start_threads(struct udpif *, size_t n_handlers, - size_t n_revalidators); +static void udpif_start_threads(struct udpif *, uint32_t n_handlers, + uint32_t n_revalidators); static void udpif_pause_revalidators(struct udpif *); static void udpif_resume_revalidators(struct udpif *); static void *udpif_upcall_handler(void *); @@ -522,7 +522,7 @@ static void udpif_stop_threads(struct udpif *udpif, bool delete_flows) { if (udpif && (udpif->n_handlers != 0 || udpif->n_revalidators != 0)) { - size_t i; + uint32_t i; /* Tell the threads to exit. */ latch_set(&udpif->exit_latch); @@ -562,8 +562,8 @@ udpif_stop_threads(struct udpif *udpif, bool delete_flows) /* Starts the handler and revalidator threads. */ static void -udpif_start_threads(struct udpif *udpif, size_t n_handlers_, - size_t n_revalidators_) +udpif_start_threads(struct udpif *udpif, uint32_t n_handlers_, + uint32_t n_revalidators_) { if (udpif && n_handlers_ && n_revalidators_) { /* Creating a thread can take a significant amount of time on some @@ -574,7 +574,7 @@ udpif_start_threads(struct udpif *udpif, size_t n_handlers_, udpif->n_revalidators = n_revalidators_; udpif->handlers = xzalloc(udpif->n_handlers * sizeof *udpif->handlers); - for (size_t i = 0; i < udpif->n_handlers; i++) { + for (uint32_t i = 0; i < udpif->n_handlers; i++) { struct handler *handler = &udpif->handlers[i]; handler->udpif = udpif; @@ -632,8 +632,8 @@ udpif_resume_revalidators(struct udpif *udpif) * datapath handle must have packet reception enabled before starting * threads. */ void -udpif_set_threads(struct udpif *udpif, size_t n_handlers_, - size_t n_revalidators_) +udpif_set_threads(struct udpif *udpif, uint32_t n_handlers_, + uint32_t n_revalidators_) { ovs_assert(udpif); ovs_assert(n_handlers_ && n_revalidators_); @@ -691,8 +691,8 @@ udpif_get_memory_usage(struct udpif *udpif, struct simap *usage) void udpif_flush(struct udpif *udpif) { - size_t n_handlers_ = udpif->n_handlers; - size_t n_revalidators_ = udpif->n_revalidators; + uint32_t n_handlers_ = udpif->n_handlers; + uint32_t n_revalidators_ = udpif->n_revalidators; udpif_stop_threads(udpif, true); dpif_flow_flush(udpif->dpif); diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h index 693107ae56c1..b4dfed32046e 100644 --- a/ofproto/ofproto-dpif-upcall.h +++ b/ofproto/ofproto-dpif-upcall.h @@ -16,6 +16,7 @@ #define OFPROTO_DPIF_UPCALL_H #include <stddef.h> +#include <inttypes.h> struct dpif; struct dpif_backer; @@ -31,8 +32,8 @@ struct simap; void udpif_init(void); struct udpif *udpif_create(struct dpif_backer *, struct dpif *); void udpif_run(struct udpif *udpif); -void udpif_set_threads(struct udpif *, size_t n_handlers, - size_t n_revalidators); +void udpif_set_threads(struct udpif *, uint32_t n_handlers, + uint32_t n_revalidators); void udpif_destroy(struct udpif *); void udpif_revalidate(struct udpif *); void udpif_get_memory_usage(struct udpif *, struct simap *usage); diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 9ad2b71d23eb..57c7d17cb28f 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -534,7 +534,7 @@ extern unsigned ofproto_min_revalidate_pps; /* Number of upcall handler and revalidator threads. Only affects the * ofproto-dpif implementation. */ -extern size_t n_handlers, n_revalidators; +extern uint32_t n_handlers, n_revalidators; static inline struct rule *rule_from_cls_rule(const struct cls_rule *); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index b91517cd250d..69e5834e766c 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -309,7 +309,7 @@ unsigned ofproto_max_idle = OFPROTO_MAX_IDLE_DEFAULT; unsigned ofproto_max_revalidator = OFPROTO_MAX_REVALIDATOR_DEFAULT; unsigned ofproto_min_revalidate_pps = OFPROTO_MIN_REVALIDATE_PPS_DEFAULT; -size_t n_handlers, n_revalidators; +uint32_t n_handlers, n_revalidators; /* Map from datapath name to struct ofproto, for use by unixctl commands. */ static struct hmap all_ofprotos = HMAP_INITIALIZER(&all_ofprotos);
'n_handlers' and 'n_revalidators' are declared as type 'size_t'. However, dpif_handlers_set() requires parameter 'n_handlers' as type 'uint32_t'. This patch fixes this type mismatch. Signed-off-by: Mark Gray <mark.d.gray@redhat.com> --- ofproto/ofproto-dpif-upcall.c | 24 ++++++++++++------------ ofproto/ofproto-dpif-upcall.h | 5 +++-- ofproto/ofproto-provider.h | 2 +- ofproto/ofproto.c | 2 +- 4 files changed, 17 insertions(+), 16 deletions(-)