[v2,06/15] event/sw: switch test counter to dynamic mbuf field
diff mbox series

Message ID 20201026222013.2147904-7-thomas@monjalon.net
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • remove mbuf userdata
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Oct. 26, 2020, 10:20 p.m. UTC
The test worker_loopback used the deprecated mbuf field udata64.
It is moved to a dynamic field in order to allow removal of udata64.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/event/sw/sw_evdev_selftest.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Comments

Olivier Matz Oct. 27, 2020, 10:15 a.m. UTC | #1
On Mon, Oct 26, 2020 at 11:20:04PM +0100, Thomas Monjalon wrote:
> The test worker_loopback used the deprecated mbuf field udata64.
> It is moved to a dynamic field in order to allow removal of udata64.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
>  drivers/event/sw/sw_evdev_selftest.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
> index 5c7e527917..9af20cecf1 100644
> --- a/drivers/event/sw/sw_evdev_selftest.c
> +++ b/drivers/event/sw/sw_evdev_selftest.c
> @@ -40,6 +40,11 @@ struct test {
>  	uint32_t service_id;
>  };
>  
> +static int counter_dynfield_offset;

In general, I wonder if we shouldn't initialize offset to -1.


> +#define COUNTER_FIELD_TYPE uint8_t

Another general comment, I suggest to use a typedef instead of
a define when relevant.


> +#define COUNTER_FIELD(mbuf) (*RTE_MBUF_DYNFIELD(mbuf, \
> +		counter_dynfield_offset, COUNTER_FIELD_TYPE *))
> +

I'm not sure this comment applies here, but since it's a simple example,
it's a good place for another general comment. The RTE_MBUF_DYNFIELD()
macro is convenient because it can be used to set or get a value of any
type, but in my opinion it is not always easy to read:

  RTE_MBUF_DYNFIELD(m, off, type) = value;

In some situations, having wrappers may make the code more readable:

  static inline void mbuf_set_counter(struct rte_mbuf *m, counter_field_t counter);
  static inline counter_field_t mbuf_get_counter(struct rte_mbuf *m);
  static inline void mbuf_inc_counter(struct rte_mbuf *m);

>  static struct rte_event release_ev;
>  
>  static inline struct rte_mbuf *
> @@ -2987,8 +2992,8 @@ worker_loopback_worker_fn(void *arg)
>  			}
>  
>  			ev[i].queue_id = 0;
> -			ev[i].mbuf->udata64++;
> -			if (ev[i].mbuf->udata64 != 16) {
> +			COUNTER_FIELD(ev[i].mbuf)++;
> +			if (COUNTER_FIELD(ev[i].mbuf) != 16) {
>  				ev[i].op = RTE_EVENT_OP_FORWARD;
>  				enqd = rte_event_enqueue_burst(evdev, port,
>  						&ev[i], 1);
> @@ -3028,7 +3033,7 @@ worker_loopback_producer_fn(void *arg)
>  			m = rte_pktmbuf_alloc(t->mbuf_pool);
>  		} while (m == NULL);
>  
> -		m->udata64 = 0;
> +		COUNTER_FIELD(m) = 0;
>  
>  		struct rte_event ev = {
>  				.op = RTE_EVENT_OP_NEW,
> @@ -3061,6 +3066,18 @@ worker_loopback(struct test *t, uint8_t disable_implicit_release)
>  	int err;
>  	int w_lcore, p_lcore;
>  
> +	static const struct rte_mbuf_dynfield counter_dynfield_desc = {
> +		.name = "rte_event_sw_dynfield_selftest_counter",
> +		.size = sizeof(COUNTER_FIELD_TYPE),
> +		.align = __alignof__(COUNTER_FIELD_TYPE),
> +	};
> +	counter_dynfield_offset =
> +		rte_mbuf_dynfield_register(&counter_dynfield_desc);
> +	if (counter_dynfield_offset < 0) {
> +		printf("Error registering mbuf field\n");
> +		return -rte_errno;
> +	}
> +
>  	if (init(t, 8, 2) < 0 ||
>  			create_atomic_qids(t, 8) < 0) {
>  		printf("%d: Error initializing device\n", __LINE__);
> -- 
> 2.28.0
>
Thomas Monjalon Oct. 27, 2020, 4:14 p.m. UTC | #2
27/10/2020 11:15, Olivier Matz:
> On Mon, Oct 26, 2020 at 11:20:04PM +0100, Thomas Monjalon wrote:
> > The test worker_loopback used the deprecated mbuf field udata64.
> > It is moved to a dynamic field in order to allow removal of udata64.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > ---
[...]
> > +static int counter_dynfield_offset;
> 
> In general, I wonder if we shouldn't initialize offset to -1.

Yes good idea.

> > +#define COUNTER_FIELD_TYPE uint8_t
> 
> Another general comment, I suggest to use a typedef instead of
> a define when relevant.

Yes

> > +#define COUNTER_FIELD(mbuf) (*RTE_MBUF_DYNFIELD(mbuf, \
> > +		counter_dynfield_offset, COUNTER_FIELD_TYPE *))
> > +
> 
> I'm not sure this comment applies here, but since it's a simple example,
> it's a good place for another general comment. The RTE_MBUF_DYNFIELD()
> macro is convenient because it can be used to set or get a value of any
> type, but in my opinion it is not always easy to read:
> 
>   RTE_MBUF_DYNFIELD(m, off, type) = value;
> 
> In some situations, having wrappers may make the code more readable:
> 
>   static inline void mbuf_set_counter(struct rte_mbuf *m, counter_field_t counter);
>   static inline counter_field_t mbuf_get_counter(struct rte_mbuf *m);
>   static inline void mbuf_inc_counter(struct rte_mbuf *m);

I agree, I will add some wrapper functions.

Patch
diff mbox series

diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index 5c7e527917..9af20cecf1 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -40,6 +40,11 @@  struct test {
 	uint32_t service_id;
 };
 
+static int counter_dynfield_offset;
+#define COUNTER_FIELD_TYPE uint8_t
+#define COUNTER_FIELD(mbuf) (*RTE_MBUF_DYNFIELD(mbuf, \
+		counter_dynfield_offset, COUNTER_FIELD_TYPE *))
+
 static struct rte_event release_ev;
 
 static inline struct rte_mbuf *
@@ -2987,8 +2992,8 @@  worker_loopback_worker_fn(void *arg)
 			}
 
 			ev[i].queue_id = 0;
-			ev[i].mbuf->udata64++;
-			if (ev[i].mbuf->udata64 != 16) {
+			COUNTER_FIELD(ev[i].mbuf)++;
+			if (COUNTER_FIELD(ev[i].mbuf) != 16) {
 				ev[i].op = RTE_EVENT_OP_FORWARD;
 				enqd = rte_event_enqueue_burst(evdev, port,
 						&ev[i], 1);
@@ -3028,7 +3033,7 @@  worker_loopback_producer_fn(void *arg)
 			m = rte_pktmbuf_alloc(t->mbuf_pool);
 		} while (m == NULL);
 
-		m->udata64 = 0;
+		COUNTER_FIELD(m) = 0;
 
 		struct rte_event ev = {
 				.op = RTE_EVENT_OP_NEW,
@@ -3061,6 +3066,18 @@  worker_loopback(struct test *t, uint8_t disable_implicit_release)
 	int err;
 	int w_lcore, p_lcore;
 
+	static const struct rte_mbuf_dynfield counter_dynfield_desc = {
+		.name = "rte_event_sw_dynfield_selftest_counter",
+		.size = sizeof(COUNTER_FIELD_TYPE),
+		.align = __alignof__(COUNTER_FIELD_TYPE),
+	};
+	counter_dynfield_offset =
+		rte_mbuf_dynfield_register(&counter_dynfield_desc);
+	if (counter_dynfield_offset < 0) {
+		printf("Error registering mbuf field\n");
+		return -rte_errno;
+	}
+
 	if (init(t, 8, 2) < 0 ||
 			create_atomic_qids(t, 8) < 0) {
 		printf("%d: Error initializing device\n", __LINE__);