[dpdk-dev,v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage
Checks
Commit Message
Signed-off-by: Hanoh Haim <hhaim@cisco.com>
---
lib/librte_mbuf/rte_mbuf.h | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
Comments
> On Nov 15, 2017, at 1:14 PM, Hanoh Haim <hhaim@cisco.com> wrote:
>
> Signed-off-by: Hanoh Haim <hhaim@cisco.com>
> ---
> lib/librte_mbuf/rte_mbuf.h | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 7e326bb..ab110f8 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1159,6 +1159,15 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
> __rte_mbuf_sanity_check(m, 1);
> }
>
> +
> +static __rte_always_inline void rte_pktmbuf_reset_before_free(struct rte_mbuf *m)
> +{
> + if (m->next != NULL) {
> + m->next = NULL;
> + m->nb_segs = 1;
> + }
> +}
> +
Probably it will be more clean to add something __te_pktmbuf_reset_nb_segs() without
check for (m->next != NULL) and use it everywhere in mbuf’s the code, not only in
rte_pktmbuf_prefree_seg() function. And I think it will be better to have separate
patch for that.
> /**
> * Allocate a new mbuf from a mempool.
> *
> @@ -1323,8 +1332,7 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
> m->ol_flags = 0;
>
> if (rte_mbuf_refcnt_update(md, -1) == 0) {
> - md->next = NULL;
> - md->nb_segs = 1;
Using rte_pktmbuf_reset_before_free() here adds unnecessary check for m->next
in that path.
> + rte_pktmbuf_reset_before_free(md);
> rte_mbuf_refcnt_set(md, 1);
> rte_mbuf_raw_free(md);
> }
> @@ -1354,25 +1362,16 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> if (RTE_MBUF_INDIRECT(m))
> rte_pktmbuf_detach(m);
>
> - if (m->next != NULL) {
> - m->next = NULL;
> - m->nb_segs = 1;
> - }
> -
> + rte_pktmbuf_reset_before_free(m);
> return m;
>
> - } else if (rte_atomic16_add_return(&m->refcnt_atomic, -1) == 0) {
> -
> + } else if (rte_mbuf_refcnt_update(m, -1) == 0) {
>
> if (RTE_MBUF_INDIRECT(m))
> rte_pktmbuf_detach(m);
>
> - if (m->next != NULL) {
> - m->next = NULL;
> - m->nb_segs = 1;
> - }
> + rte_pktmbuf_reset_before_free(m);
> rte_mbuf_refcnt_set(m, 1);
> -
> return m;
> }
> return NULL;
> --
> 2.9.3
>
+Oliver,
Ilia, I assume there is a reason why Oliver did that, I just consolidate the code.
He didn't want to *write* the same value to mbuf field.
In the common case that pointer was already cleared by the driver, it is better to just read it. From cache synchronization perspective it will run faster.
Thanks,
Hanoh
-----Original Message-----
From: Ilya Matveychikov [mailto:matvejchikov@gmail.com]
Sent: Wednesday, November 15, 2017 1:14 PM
To: Hanoch Haim (hhaim)
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage
> On Nov 15, 2017, at 1:14 PM, Hanoh Haim <hhaim@cisco.com> wrote:
>
> Signed-off-by: Hanoh Haim <hhaim@cisco.com>
> ---
> lib/librte_mbuf/rte_mbuf.h | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 7e326bb..ab110f8 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1159,6 +1159,15 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
> __rte_mbuf_sanity_check(m, 1);
> }
>
> +
> +static __rte_always_inline void rte_pktmbuf_reset_before_free(struct
> +rte_mbuf *m) {
> + if (m->next != NULL) {
> + m->next = NULL;
> + m->nb_segs = 1;
> + }
> +}
> +
Probably it will be more clean to add something __te_pktmbuf_reset_nb_segs() without check for (m->next != NULL) and use it everywhere in mbuf’s the code, not only in
rte_pktmbuf_prefree_seg() function. And I think it will be better to have separate patch for that.
> /**
> * Allocate a new mbuf from a mempool.
> *
> @@ -1323,8 +1332,7 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
> m->ol_flags = 0;
>
> if (rte_mbuf_refcnt_update(md, -1) == 0) {
> - md->next = NULL;
> - md->nb_segs = 1;
Using rte_pktmbuf_reset_before_free() here adds unnecessary check for m->next in that path.
> + rte_pktmbuf_reset_before_free(md);
> rte_mbuf_refcnt_set(md, 1);
> rte_mbuf_raw_free(md);
> }
> @@ -1354,25 +1362,16 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> if (RTE_MBUF_INDIRECT(m))
> rte_pktmbuf_detach(m);
>
> - if (m->next != NULL) {
> - m->next = NULL;
> - m->nb_segs = 1;
> - }
> -
> + rte_pktmbuf_reset_before_free(m);
> return m;
>
> - } else if (rte_atomic16_add_return(&m->refcnt_atomic, -1) == 0) {
> -
> + } else if (rte_mbuf_refcnt_update(m, -1) == 0) {
>
> if (RTE_MBUF_INDIRECT(m))
> rte_pktmbuf_detach(m);
>
> - if (m->next != NULL) {
> - m->next = NULL;
> - m->nb_segs = 1;
> - }
> + rte_pktmbuf_reset_before_free(m);
> rte_mbuf_refcnt_set(m, 1);
> -
> return m;
> }
> return NULL;
> --
> 2.9.3
>
@@ -1159,6 +1159,15 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
__rte_mbuf_sanity_check(m, 1);
}
+
+static __rte_always_inline void rte_pktmbuf_reset_before_free(struct rte_mbuf *m)
+{
+ if (m->next != NULL) {
+ m->next = NULL;
+ m->nb_segs = 1;
+ }
+}
+
/**
* Allocate a new mbuf from a mempool.
*
@@ -1323,8 +1332,7 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
m->ol_flags = 0;
if (rte_mbuf_refcnt_update(md, -1) == 0) {
- md->next = NULL;
- md->nb_segs = 1;
+ rte_pktmbuf_reset_before_free(md);
rte_mbuf_refcnt_set(md, 1);
rte_mbuf_raw_free(md);
}
@@ -1354,25 +1362,16 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
if (RTE_MBUF_INDIRECT(m))
rte_pktmbuf_detach(m);
- if (m->next != NULL) {
- m->next = NULL;
- m->nb_segs = 1;
- }
-
+ rte_pktmbuf_reset_before_free(m);
return m;
- } else if (rte_atomic16_add_return(&m->refcnt_atomic, -1) == 0) {
-
+ } else if (rte_mbuf_refcnt_update(m, -1) == 0) {
if (RTE_MBUF_INDIRECT(m))
rte_pktmbuf_detach(m);
- if (m->next != NULL) {
- m->next = NULL;
- m->nb_segs = 1;
- }
+ rte_pktmbuf_reset_before_free(m);
rte_mbuf_refcnt_set(m, 1);
-
return m;
}
return NULL;