[dpdk-dev] [PATCH] mempool: fix check flags
Sorry,
I did not notice the same kind of patch.
I close this patch.
Hiroyuki
2016-09-08 23:46 GMT+09:00 Olivier Matz :
> Hi Hiroki, Ferruh,
>
> On 09/08/2016 04:44 PM, Ferruh Yigit wrote:
>> On 9/8/2016 3:28 PM, Hiroyuki Mikita wrote:
>>> fix check flags in case of single producer and single consumer
>>>
>>> Fixes: 449c49b9 ("mempool: support handler operations")
>>>
>>> Signed-off-by: Hiroyuki Mikita
>>> ---
>>> lib/librte_mempool/rte_mempool.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_mempool/rte_mempool.c
>>> b/lib/librte_mempool/rte_mempool.c
>>> index 2e28e2e..61bd63c 100644
>>> --- a/lib/librte_mempool/rte_mempool.c
>>> +++ b/lib/librte_mempool/rte_mempool.c
>>> @@ -879,7 +879,7 @@ rte_mempool_create(const char *name, unsigned n,
>>> unsigned elt_size,
>>> * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
>>> * set the correct index into the table of ops structs.
>>> */
>>> -if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
>>> +if ((flags & MEMPOOL_F_SP_PUT) & (flags & MEMPOOL_F_SC_GET))
>>
>> Isn't this always false?
>>
>> What about:
>> if ((flags & MEMPOOL_F_SP_PUT) && (flags & MEMPOOL_F_SC_GET))
>>
>>> rte_mempool_set_ops_byname(mp, "ring_sp_sc", NULL);
>>> else if (flags & MEMPOOL_F_SP_PUT)
>>> rte_mempool_set_ops_byname(mp, "ring_sp_mc", NULL);
>>>
>>
>
> Looks the same kind of patch was posted few hours before:
> http://dpdk.org/dev/patchwork/patch/15686/
>
> Regards,
> Olivier
[dpdk-dev] [PATCH] mempool: fix check flags
fix check flags in case of single producer and single consumer
Fixes: 449c49b9 ("mempool: support handler operations")
Signed-off-by: Hiroyuki Mikita
---
lib/librte_mempool/rte_mempool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 2e28e2e..61bd63c 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -879,7 +879,7 @@ rte_mempool_create(const char *name, unsigned n, unsigned
elt_size,
* Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
* set the correct index into the table of ops structs.
*/
- if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
+ if ((flags & MEMPOOL_F_SP_PUT) & (flags & MEMPOOL_F_SC_GET))
rte_mempool_set_ops_byname(mp, "ring_sp_sc", NULL);
else if (flags & MEMPOOL_F_SP_PUT)
rte_mempool_set_ops_byname(mp, "ring_sp_mc", NULL);
--
2.7.4
[dpdk-dev] [PATCH v2] sched: fix releasing enqueued packets
rte_sched_port_free should release only enqueued packets of all queues.
Previous behavior is that enqueued and already dequeued packets of
only first 4 queues are released.
Fixes: 61383240 ("sched: release enqueued mbufs when freeing port")
Signed-off-by: Hiroyuki Mikita
---
v2:
* use rte_sched_port_queues_per_port to get the number of queues
* mask incremented qr by qsize - 1
lib/librte_sched/rte_sched.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 8696423..e6dace2 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -734,19 +734,23 @@ rte_sched_port_config(struct rte_sched_port_params
*params)
void
rte_sched_port_free(struct rte_sched_port *port)
{
- unsigned int queue;
+ uint32_t qindex;
+ uint32_t n_queues_per_port = rte_sched_port_queues_per_port(port);
/* Check user parameters */
if (port == NULL)
return;
/* Free enqueued mbufs */
- for (queue = 0; queue < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; queue++) {
- struct rte_mbuf **mbufs = rte_sched_port_qbase(port, queue);
- unsigned int i;
-
- for (i = 0; i < rte_sched_port_qsize(port, queue); i++)
- rte_pktmbuf_free(mbufs[i]);
+ for (qindex = 0; qindex < n_queues_per_port; qindex++) {
+ struct rte_mbuf **mbufs = rte_sched_port_qbase(port, qindex);
+ uint16_t qsize = rte_sched_port_qsize(port, qindex);
+ struct rte_sched_queue *queue = port->queue + qindex;
+ uint16_t qr = queue->qr & (qsize - 1);
+ uint16_t qw = queue->qw & (qsize - 1);
+
+ for (; qr != qw; qr = (qr + 1) & (qsize - 1))
+ rte_pktmbuf_free(mbufs[qr]);
}
rte_bitmap_free(port->bmp);
--
2.7.4
[dpdk-dev] [PATCH] sched: fix releasing enqueued packets
rte_sched_port_free should release only enqueued packets of all queues.
Previous behavior is that enqueued and already dequeued packets of
only first 4 queues are released.
Fixes: 61383240 ("sched: release enqueued mbufs when freeing port")
Signed-off-by: Hiroyuki Mikita
---
lib/librte_sched/rte_sched.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 8696423..371003e 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -734,19 +734,24 @@ rte_sched_port_config(struct rte_sched_port_params
*params)
void
rte_sched_port_free(struct rte_sched_port *port)
{
- unsigned int queue;
+ uint32_t qindex;
+ uint32_t n_queues_per_port = RTE_SCHED_QUEUES_PER_PIPE *
+ port->n_pipes_per_subport * port->n_subports_per_port;
/* Check user parameters */
if (port == NULL)
return;
/* Free enqueued mbufs */
- for (queue = 0; queue < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; queue++) {
- struct rte_mbuf **mbufs = rte_sched_port_qbase(port, queue);
- unsigned int i;
-
- for (i = 0; i < rte_sched_port_qsize(port, queue); i++)
- rte_pktmbuf_free(mbufs[i]);
+ for (qindex = 0; qindex < n_queues_per_port; qindex++) {
+ struct rte_mbuf **mbufs = rte_sched_port_qbase(port, qindex);
+ uint16_t qsize = rte_sched_port_qsize(port, qindex);
+ struct rte_sched_queue *queue = port->queue + qindex;
+ uint16_t qr = queue->qr & (qsize - 1);
+ uint16_t qw = queue->qw & (qsize - 1);
+
+ for (; qr != qw; qr++)
+ rte_pktmbuf_free(mbufs[qr]);
}
rte_bitmap_free(port->bmp);
--
2.7.4
[dpdk-dev] [PATCH] timer: fix incorrect pending-list manipulation
Fixes: 9b15ba895b9f ("timer: use a skip list")
2016-07-23 7:05 GMT+09:00 Sanford, Robert :
>
>
> On 7/17/16 10:35 AM, "Hiroyuki Mikita" wrote:
>
>>This commit fixes incorrect pending-list manipulation
>>when getting list of expired timers in rte_timer_manage().
>>
>>When timer_get_prev_entries() sets pending_head on prev,
>>the pending-list is broken.
>>The next of pending_head always becomes NULL.
>>In this depth level, it is not need to manipulate the list.
>>
>>Signed-off-by: Hiroyuki Mikita
>>---
>> lib/librte_timer/rte_timer.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>>diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
>>index 3dcdab5..7457d32 100644
>>--- a/lib/librte_timer/rte_timer.c
>>+++ b/lib/librte_timer/rte_timer.c
>>@@ -543,6 +543,8 @@ void rte_timer_manage(void)
>> /* break the existing list at current time point */
>> timer_get_prev_entries(cur_time, lcore_id, prev);
>> for (i = priv_timer[lcore_id].curr_skiplist_depth -1; i >= 0; i--) {
>>+ if (prev[i] == _timer[lcore_id].pending_head)
>>+ continue;
>> priv_timer[lcore_id].pending_head.sl_next[i] =
>> prev[i]->sl_next[i];
>> if (prev[i]->sl_next[i] == NULL)
>>--
>>2.7.4
>>
>
> Acked-by: Robert Sanford
>
[dpdk-dev] [PATCH] timer: remove unnecessary timer add call
Fixes: a4b7a5a45cf5 ("timer: fix race condition")
2016-07-23 7:06 GMT+09:00 Sanford, Robert :
>
>
> On 7/17/16 1:35 PM, "Hiroyuki Mikita" wrote:
>
>>When timer_set_running_state() fails in rte_timer_manage(),
>>the failed timer is put back on pending-list.
>>In this case, another core tries to reset or stop the timer.
>>It does not need to be on pending-list
>>
>>Signed-off-by: Hiroyuki Mikita
>>---
>> lib/librte_timer/rte_timer.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>>diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
>>index 3dcdab5..94878d3 100644
>>--- a/lib/librte_timer/rte_timer.c
>>+++ b/lib/librte_timer/rte_timer.c
>>@@ -562,10 +562,9 @@ void rte_timer_manage(void)
>> pprev = >sl_next[0];
>> } else {
>> /* another core is trying to re-config this one,
>>- * remove it from local expired list and put it
>>- * back on the priv_timer[] skip list */
>>+ * remove it from local expired list
>>+ */
>> *pprev = next_tim;
>>- timer_add(tim, lcore_id, 1);
>> }
>> }
>>
>>--
>>2.7.4
>>
>
> Acked-by: Robert Sanford
>
[dpdk-dev] [PATCH] timer: fix break list when timer_cb reset running timer
Fixes: a4b7a5a45cf5 ("timer: fix race condition")
2016-07-25 23:43 GMT+09:00 Thomas Monjalon :
> Hiroyuki, Robert,
> I would like to apply these patches quickly.
> Please could you provide some "Fixes:" line to know the origin
> of the bugs?
> Thanks
>
[dpdk-dev] [PATCH] timer: fix break list when timer_cb reset running timer
Hi Robert, My application had timers which reset another timer and sometimes did not work. Its profile by 'perf' command showed timer_add occupied 99.9% CPU. It seemed that an infinite loop occurred in rte_timer. I inspected timer codes and found that the main cause was a bug of the patch 3. In this process, I also found the other bugs. Regards, Hiroyuki
[dpdk-dev] [PATCH] timer: fix break list when timer_cb reset running timer
Hi Robert, Thank you for reviewing. In the following case, the skip list is broken. - Timer A and timer B are configured on the same lcore, in the same pending list. - The expire time of timer A is earlier than that of timer B. - rte_timer_manage() is called on the lcore after the expire time of timer B. - The callback of timer A resets timer B. In rte_timer_manage() process, both timers are expired at the same time, so the running list includes both timers. States of both timers are transited from PENDING to RUNNING, then callbacks are executed sequentially. The callback of timer A resets timer B which is still RUNNING, so timer B is added to the pending-list. In this time, timer B is in both the running list and the pending list. It means that the running list is chained to the pending list. Both lists are broken. Regards, Hiroyuki
[dpdk-dev] [PATCH] timer: fix break list when timer_cb reset running timer
When timer_cb resets another running timer on the same lcore,
the list of expired timers is chained to the pending-list.
This commit prevents a running timer from being reset
by not its own timer_cb.
Signed-off-by: Hiroyuki Mikita
---
lib/librte_timer/rte_timer.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 3dcdab5..00e80c9 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -69,6 +69,9 @@ struct priv_timer {
unsigned prev_lcore; /**< used for lcore round robin */
+ /** running timer on this lcore now */
+ struct rte_timer *running_tim;
+
#ifdef RTE_LIBRTE_TIMER_DEBUG
/** per-lcore statistics */
struct rte_timer_debug_stats stats;
@@ -135,9 +138,12 @@ timer_set_config_state(struct rte_timer *tim,
while (success == 0) {
prev_status.u32 = tim->status.u32;
- /* timer is running on another core, exit */
+ /* timer is running on another core
+* or ready to run on local core, exit
+*/
if (prev_status.state == RTE_TIMER_RUNNING &&
- prev_status.owner != (uint16_t)lcore_id)
+ (prev_status.owner != (uint16_t)lcore_id ||
+tim != priv_timer[lcore_id].running_tim))
return -1;
/* timer is being configured on another core */
@@ -580,6 +586,7 @@ void rte_timer_manage(void)
for (tim = run_first_tim; tim != NULL; tim = next_tim) {
next_tim = tim->sl_next[0];
priv_timer[lcore_id].updated = 0;
+ priv_timer[lcore_id].running_tim = tim;
/* execute callback function with list unlocked */
tim->f(tim, tim->arg);
@@ -610,6 +617,7 @@ void rte_timer_manage(void)
rte_spinlock_unlock(_timer[lcore_id].list_lock);
}
}
+ priv_timer[lcore_id].running_tim = NULL;
}
/* dump statistics about timers */
--
2.7.4
[dpdk-dev] [PATCH] timer: remove unnecessary timer add call
When timer_set_running_state() fails in rte_timer_manage(),
the failed timer is put back on pending-list.
In this case, another core tries to reset or stop the timer.
It does not need to be on pending-list
Signed-off-by: Hiroyuki Mikita
---
lib/librte_timer/rte_timer.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 3dcdab5..94878d3 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -562,10 +562,9 @@ void rte_timer_manage(void)
pprev = >sl_next[0];
} else {
/* another core is trying to re-config this one,
-* remove it from local expired list and put it
-* back on the priv_timer[] skip list */
+* remove it from local expired list
+*/
*pprev = next_tim;
- timer_add(tim, lcore_id, 1);
}
}
--
2.7.4
[dpdk-dev] [PATCH] timer: fix incorrect pending-list manipulation
This commit fixes incorrect pending-list manipulation
when getting list of expired timers in rte_timer_manage().
When timer_get_prev_entries() sets pending_head on prev,
the pending-list is broken.
The next of pending_head always becomes NULL.
In this depth level, it is not need to manipulate the list.
Signed-off-by: Hiroyuki Mikita
---
lib/librte_timer/rte_timer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 3dcdab5..7457d32 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -543,6 +543,8 @@ void rte_timer_manage(void)
/* break the existing list at current time point */
timer_get_prev_entries(cur_time, lcore_id, prev);
for (i = priv_timer[lcore_id].curr_skiplist_depth -1; i >= 0; i--) {
+ if (prev[i] == _timer[lcore_id].pending_head)
+ continue;
priv_timer[lcore_id].pending_head.sl_next[i] =
prev[i]->sl_next[i];
if (prev[i]->sl_next[i] == NULL)
--
2.7.4
[dpdk-dev] [PATCH] ip_frag: fix doxygen formatting
This commit fixes some functions missing in API documentation.
Signed-off-by: Hiroyuki Mikita
---
lib/librte_ip_frag/rte_ip_frag.h | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
index 92cedf2..f8ed526 100644
--- a/lib/librte_ip_frag/rte_ip_frag.h
+++ b/lib/librte_ip_frag/rte_ip_frag.h
@@ -77,7 +77,7 @@ struct ip_frag_key {
uint32_t key_len; /**< src/dst key length */
};
-/*
+/**
* @internal Fragmented packet to reassemble.
* First two entries in the frags[] array are for the last and first fragments.
*/
@@ -151,7 +151,7 @@ struct ipv6_extension_fragment {
-/*
+/**
* Create a new IP fragmentation table.
*
* @param bucket_num
@@ -174,7 +174,7 @@ struct rte_ip_frag_tbl * rte_ip_frag_table_create(uint32_t
bucket_num,
uint32_t bucket_entries, uint32_t max_entries,
uint64_t max_cycles, int socket_id);
-/*
+/**
* Free allocated IP fragmentation table.
*
* @param btl
@@ -215,7 +215,7 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
struct rte_mempool *pool_direct,
struct rte_mempool *pool_indirect);
-/*
+/**
* This function implements reassembly of fragmented IPv6 packets.
* Incoming mbuf should have its l2_len/l3_len fields setup correctly.
*
@@ -241,7 +241,7 @@ struct rte_mbuf *rte_ipv6_frag_reassemble_packet(struct
rte_ip_frag_tbl *tbl,
struct rte_mbuf *mb, uint64_t tms, struct ipv6_hdr *ip_hdr,
struct ipv6_extension_fragment *frag_hdr);
-/*
+/**
* Return a pointer to the packet's fragment header, if found.
* It only looks at the extension header that's right after the fixed IPv6
* header, and doesn't follow the whole chain of extension headers.
@@ -291,7 +291,7 @@ int32_t rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
struct rte_mempool *pool_direct,
struct rte_mempool *pool_indirect);
-/*
+/**
* This function implements reassembly of fragmented IPv4 packets.
* Incoming mbufs should have its l2_len/l3_len fields setup correclty.
*
@@ -314,7 +314,7 @@ struct rte_mbuf * rte_ipv4_frag_reassemble_packet(struct
rte_ip_frag_tbl *tbl,
struct rte_ip_frag_death_row *dr,
struct rte_mbuf *mb, uint64_t tms, struct ipv4_hdr *ip_hdr);
-/*
+/**
* Check if the IPv4 packet is fragmented
*
* @param hdr
@@ -333,7 +333,7 @@ rte_ipv4_frag_pkt_is_fragmented(const struct ipv4_hdr *
hdr) {
return ip_flag != 0 || ip_ofs != 0;
}
-/*
+/**
* Free mbufs on a given death row.
*
* @param dr
@@ -345,7 +345,7 @@ void rte_ip_frag_free_death_row(struct
rte_ip_frag_death_row *dr,
uint32_t prefetch);
-/*
+/**
* Dump fragmentation table statistics to file.
*
* @param f
--
2.7.4
[dpdk-dev] [PATCH] ethdev: fix formatting of doxygen comments
This commit fixes some functions missing in API documentation. Signed-off-by: Hiroyuki Mikita --- lib/librte_ether/rte_ethdev.h | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index bd93bf6..2698c3e 100644 --- a/lib/librte_ether/rte_ethdev.h +++ b/lib/librte_ether/rte_ethdev.h @@ -2009,7 +2009,7 @@ int rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id, uint16_t nb_tx_desc, unsigned int socket_id, const struct rte_eth_txconf *tx_conf); -/* +/** * Return the NUMA socket to which an Ethernet device is connected * * @param port_id @@ -2021,7 +2021,7 @@ int rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id, */ int rte_eth_dev_socket_id(uint8_t port_id); -/* +/** * Check if port_id of device is attached * * @param port_id @@ -2032,7 +2032,7 @@ int rte_eth_dev_socket_id(uint8_t port_id); */ int rte_eth_dev_is_valid_port(uint8_t port_id); -/* +/** * Allocate mbuf from mempool, setup the DMA physical address * and then start RX for specified queue of a port. It is used * when rx_deferred_start flag of the specified queue is true. @@ -2050,7 +2050,7 @@ int rte_eth_dev_is_valid_port(uint8_t port_id); */ int rte_eth_dev_rx_queue_start(uint8_t port_id, uint16_t rx_queue_id); -/* +/** * Stop specified RX queue of a port * * @param port_id @@ -2066,7 +2066,7 @@ int rte_eth_dev_rx_queue_start(uint8_t port_id, uint16_t rx_queue_id); */ int rte_eth_dev_rx_queue_stop(uint8_t port_id, uint16_t rx_queue_id); -/* +/** * Start TX for specified queue of a port. It is used when tx_deferred_start * flag of the specified queue is true. * @@ -2083,7 +2083,7 @@ int rte_eth_dev_rx_queue_stop(uint8_t port_id, uint16_t rx_queue_id); */ int rte_eth_dev_tx_queue_start(uint8_t port_id, uint16_t tx_queue_id); -/* +/** * Stop specified TX queue of a port * * @param port_id @@ -4030,7 +4030,7 @@ int rte_eth_rx_queue_info_get(uint8_t port_id, uint16_t queue_id, int rte_eth_tx_queue_info_get(uint8_t port_id, uint16_t queue_id, struct rte_eth_txq_info *qinfo); -/* +/** * Retrieve number of available registers for access * * @param port_id -- 1.9.1
[dpdk-dev] [PATCH v3] e1000: fix build with clang
GCC_VERSION is empty in case of clang:
/bin/sh: line 0: test: -ge: unary operator expected
It is the same issue as http://dpdk.org/dev/patchwork/patch/5994/
Fixes: 366113dbfb69 ("e1000: suppress misleading indentation warning")
Signed-off-by: Hiroyuki Mikita
---
v3:
* use CONFIG_RTE_TOOLCHAIN_GCC
v2:
* fix for cross compier
drivers/net/e1000/Makefile | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/e1000/Makefile b/drivers/net/e1000/Makefile
index f4879e6..b5b803e 100644
--- a/drivers/net/e1000/Makefile
+++ b/drivers/net/e1000/Makefile
@@ -50,14 +50,16 @@ ifeq ($(CC), icc)
CFLAGS_BASE_DRIVER = -wd177 -wd181 -wd188 -wd869 -wd2259
else
#
-# CFLAGS for gcc
+# CFLAGS for gcc/clang
#
CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter
CFLAGS_BASE_DRIVER += -Wno-unused-variable
+ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y)
ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1)
CFLAGS_BASE_DRIVER += -Wno-misleading-indentation
endif
endif
+endif
#
# Add extra flags for base driver files (also known as shared code)
--
1.9.1
[dpdk-dev] [PATCH v2] e1000: fix build with clang
Hi Thomas, > The output of git grep '(CC)' shows that there is some room for > cross-compilation fixes. > Any volunteer? > OK. I will send another patch after merging this. This is just a fix for clang build. Regards, Hiroyuki
[dpdk-dev] [PATCH] e1000: fix build with clang
GCC_VERSION is empty in case of clang:
/bin/sh: line 0: test: -ge: unary operator expected
It is the same issue as http://dpdk.org/dev/patchwork/patch/5994/
Fixes: 366113dbfb69 ("e1000: suppress misleading indentation warning")
Signed-off-by: Hiroyuki Mikita
---
drivers/net/e1000/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/e1000/Makefile b/drivers/net/e1000/Makefile
index f4879e6..d580dea 100644
--- a/drivers/net/e1000/Makefile
+++ b/drivers/net/e1000/Makefile
@@ -50,11 +50,11 @@ ifeq ($(CC), icc)
CFLAGS_BASE_DRIVER = -wd177 -wd181 -wd188 -wd869 -wd2259
else
#
-# CFLAGS for gcc
+# CFLAGS for gcc/clang
#
CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter
CFLAGS_BASE_DRIVER += -Wno-unused-variable
-ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1)
+ifeq ($(shell test $(CC) = gcc && test $(GCC_VERSION) -ge 60 && echo 1), 1)
CFLAGS_BASE_DRIVER += -Wno-misleading-indentation
endif
endif
--
1.9.1
[dpdk-dev] [PATCH v4] mbuf: decrease refcnt when detaching
The rte_pktmbuf_detach() function should decrease refcnt on a direct
buffer.
Signed-off-by: Hiroyuki Mikita
---
v4:
* fixed formatting.
* fixed doxygen comments of rte_pktmbuf_detach() to be clearer.
v3:
* fixed rte_pktmbuf_detach() to decrease refcnt.
* free the direct mbuf when refcnt becomes 0.
* added this issue to Resolved Issues in release notes.
v2:
* introduced a new function rte_pktmbuf_detach2() which decrease refcnt.
* marked rte_pktmbuf_detach() as deprecated.
* added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
* checked refcnt when detaching in unit tests.
* added this issue to release notes.
app/test/test_mbuf.c | 4
doc/guides/rel_notes/release_16_07.rst | 6 ++
lib/librte_mbuf/rte_mbuf.h | 23 ---
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 98ff93a..8460db7 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -511,10 +511,14 @@ test_attach_from_different_pool(void)
rte_pktmbuf_detach(clone);
if (c_data != rte_pktmbuf_mtod(clone, char *))
GOTO_FAIL("clone was not detached properly\n");
+ if (rte_mbuf_refcnt_read(m) != 2)
+ GOTO_FAIL("invalid refcnt in m\n");
rte_pktmbuf_detach(clone2);
if (c_data2 != rte_pktmbuf_mtod(clone2, char *))
GOTO_FAIL("clone2 was not detached properly\n");
+ if (rte_mbuf_refcnt_read(m) != 1)
+ GOTO_FAIL("invalid refcnt in m\n");
/* free the clones and the initial mbuf */
rte_pktmbuf_free(clone2);
diff --git a/doc/guides/rel_notes/release_16_07.rst
b/doc/guides/rel_notes/release_16_07.rst
index 58c8ef9..a8c50c8 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -69,6 +69,12 @@ Drivers
Libraries
~
+* **mbuf: Fixed refcnt update when detaching.**
+
+ Fix the ``rte_pktmbuf_detach()`` function to decrement the direct
+ mbuf's reference counter. The previous behavior was not to affect
+ the reference counter. It lead a memory leak of the direct mbuf.
+
Examples
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 7b92b88..48911a6 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1412,6 +1412,8 @@ static inline int rte_pktmbuf_alloc_bulk(struct
rte_mempool *pool,
*
* After attachment we refer the mbuf we attached as 'indirect',
* while mbuf we attached to as 'direct'.
+ * The direct mbuf's reference counter is incremented.
+ *
* Right now, not supported:
* - attachment for already indirect mbuf (e.g. - mi has to be direct).
* - mbuf we trying to attach (mi) is used by someone else
@@ -1465,13 +1467,17 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf
*mi, struct rte_mbuf *m)
*
* - restore original mbuf address and length values.
* - reset pktmbuf data and data_len to their default values.
- * All other fields of the given packet mbuf will be left intact.
+ * - decrement the direct mbuf's reference counter. When the
+ * reference counter becomes 0, the direct mbuf is freed.
+ *
+ * All other fields of the given packet mbuf will be left intact.
*
* @param m
* The indirect attached packet mbuf.
*/
static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
{
+ struct rte_mbuf *md = rte_mbuf_from_indirect(m);
struct rte_mempool *mp = m->pool;
uint32_t mbuf_size, buf_len, priv_size;
@@ -1486,6 +1492,9 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
m->data_len = 0;
m->ol_flags = 0;
+
+ if (rte_mbuf_refcnt_update(md, -1) == 0)
+ __rte_mbuf_raw_free(md);
}
static inline struct rte_mbuf* __attribute__((always_inline))
@@ -1494,17 +1503,9 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
__rte_mbuf_sanity_check(m, 0);
if (likely(rte_mbuf_refcnt_update(m, -1) == 0)) {
-
- /* if this is an indirect mbuf, then
-* - detach mbuf
-* - free attached mbuf segment
-*/
- if (RTE_MBUF_INDIRECT(m)) {
- struct rte_mbuf *md = rte_mbuf_from_indirect(m);
+ /* if this is an indirect mbuf, it is detached. */
+ if (RTE_MBUF_INDIRECT(m))
rte_pktmbuf_detach(m);
- if (rte_mbuf_refcnt_update(md, -1) == 0)
- __rte_mbuf_raw_free(md);
- }
return m;
}
return NULL;
--
1.9.1
[dpdk-dev] [PATCH v3] mbuf: decrease refcnt when detaching
Hi Olivier,
Thanks for reviewing.
I am fixing the patch to follow your comments.
Regards,
Hiroyuki
2016-05-18 20:58 GMT+09:00 Olivier Matz :
> Hi Hiroyuki,
>
> Thanks for submitting a new version.
>
> There are some styling issues in the patch, I highlighted them below
> but you can check them by using checkpatch:
>
> DPDK_CHECKPATCH_PATH=/path/to/linux/checkpatch.pl \
> scripts/checkpatches.sh file.patch
>
>
> On 05/17/2016 06:35 PM, Hiroyuki Mikita wrote:
>> The rte_pktmbuf_detach() function should decrease refcnt on a direct
>> buffer.
>>
>> Signed-off-by: Hiroyuki Mikita
>>
>> [...]
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 529debb..299b60e 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -1408,6 +1408,8 @@ static inline int rte_pktmbuf_alloc_bulk(struct
>> rte_mempool *pool,
>> *
>> * After attachment we refer the mbuf we attached as 'indirect',
>> * while mbuf we attached to as 'direct'.
>> + * The direct mbuf's reference counter is incremented.
>> + *
>
> ERROR:TRAILING_WHITESPACE: trailing whitespace
> #82: FILE: lib/librte_mbuf/rte_mbuf.h:1412:
> + * $
>
>
>> * Right now, not supported:
>> * - attachment for already indirect mbuf (e.g. - mi has to be direct).
>> * - mbuf we trying to attach (mi) is used by someone else
>> @@ -1462,12 +1464,15 @@ static inline void rte_pktmbuf_attach(struct
>> rte_mbuf *mi, struct rte_mbuf *m)
>> * - restore original mbuf address and length values.
>> * - reset pktmbuf data and data_len to their default values.
>> * All other fields of the given packet mbuf will be left intact.
>> + * - decrement the direct mbuf's reference counter.
>> + * When the reference counter becomes 0, the direct mbuf is freed.
>
> Minor comment here: I think something like that would be clearer:
>
> * - restore original mbuf address and length values.
> * - reset pktmbuf data and data_len to their default values.
> * - decrement the direct mbuf's reference counter. When the
> *reference counter becomes 0, the direct mbuf is freed.
> *
> * All other fields of the given packet mbuf will be left intact.
>
>
>> *
>> * @param m
>> * The indirect attached packet mbuf.
>> */
>> static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>> {
>> + struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>> struct rte_mempool *mp = m->pool;
>> uint32_t mbuf_size, buf_len, priv_size;
>>
>> @@ -1482,6 +1487,10 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf
>> *m)
>> m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
>> m->data_len = 0;
>> m->ol_flags = 0;
>> +
>> + if (rte_mbuf_refcnt_update(md, -1) == 0) {
>> + __rte_mbuf_raw_free(md);
>> + }
>> }
>
> WARNING:BRACES: braces {} are not necessary for single statement blocks
> #107: FILE: lib/librte_mbuf/rte_mbuf.h:1491:
> + if (rte_mbuf_refcnt_update(md, -1) == 0) {
> + __rte_mbuf_raw_free(md);
> + }
>
>
>>
>> static inline struct rte_mbuf* __attribute__((always_inline))
>> @@ -1491,15 +1500,9 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>
>> if (likely(rte_mbuf_refcnt_update(m, -1) == 0)) {
>>
>> - /* if this is an indirect mbuf, then
>> - * - detach mbuf
>> - * - free attached mbuf segment
>> - */
>> + /* if this is an indirect mbuf, it is detached. */
>> if (RTE_MBUF_INDIRECT(m)) {
>> - struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>> rte_pktmbuf_detach(m);
>> - if (rte_mbuf_refcnt_update(md, -1) == 0)
>> - __rte_mbuf_raw_free(md);
>> }
>> return m;
>> }
>>
>
> It's not seen by checkpatch, but the braces could also be removed
> here.
>
> Apart from this, it looks good to me. I'm ok with not providing a
> compat function as we could consider it was a bug.
>
> Thanks for working on this.
>
> Olivier
>
[dpdk-dev] [PATCH v3] mbuf: decrease refcnt when detaching
The rte_pktmbuf_detach() function should decrease refcnt on a direct
buffer.
Signed-off-by: Hiroyuki Mikita
---
v3:
* fixed rte_pktmbuf_detach() to decrease refcnt.
* free the direct mbuf when refcnt becomes 0.
* added this issue to Resolved Issues in release notes.
v2:
* introduced a new function rte_pktmbuf_detach2() which decrease refcnt.
* marked rte_pktmbuf_detach() as deprecated.
* added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
* checked refcnt when detaching in unit tests.
* added this issue to release notes.
app/test/test_mbuf.c | 4
doc/guides/rel_notes/release_16_07.rst | 6 ++
lib/librte_mbuf/rte_mbuf.h | 17 ++---
3 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 98ff93a..8460db7 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -511,10 +511,14 @@ test_attach_from_different_pool(void)
rte_pktmbuf_detach(clone);
if (c_data != rte_pktmbuf_mtod(clone, char *))
GOTO_FAIL("clone was not detached properly\n");
+ if (rte_mbuf_refcnt_read(m) != 2)
+ GOTO_FAIL("invalid refcnt in m\n");
rte_pktmbuf_detach(clone2);
if (c_data2 != rte_pktmbuf_mtod(clone2, char *))
GOTO_FAIL("clone2 was not detached properly\n");
+ if (rte_mbuf_refcnt_read(m) != 1)
+ GOTO_FAIL("invalid refcnt in m\n");
/* free the clones and the initial mbuf */
rte_pktmbuf_free(clone2);
diff --git a/doc/guides/rel_notes/release_16_07.rst
b/doc/guides/rel_notes/release_16_07.rst
index f6d543c..8283373 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -65,6 +65,12 @@ Drivers
Libraries
~
+* **mbuf: Fixed refcnt update when detaching.**
+
+ Fix the ``rte_pktmbuf_detach()`` function to decrement the direct
+ mbuf's reference counter. The previous behavior was not to affect
+ the reference counter. It lead a memory leak of the direct mbuf.
+
Examples
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 529debb..299b60e 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1408,6 +1408,8 @@ static inline int rte_pktmbuf_alloc_bulk(struct
rte_mempool *pool,
*
* After attachment we refer the mbuf we attached as 'indirect',
* while mbuf we attached to as 'direct'.
+ * The direct mbuf's reference counter is incremented.
+ *
* Right now, not supported:
* - attachment for already indirect mbuf (e.g. - mi has to be direct).
* - mbuf we trying to attach (mi) is used by someone else
@@ -1462,12 +1464,15 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf
*mi, struct rte_mbuf *m)
* - restore original mbuf address and length values.
* - reset pktmbuf data and data_len to their default values.
* All other fields of the given packet mbuf will be left intact.
+ * - decrement the direct mbuf's reference counter.
+ * When the reference counter becomes 0, the direct mbuf is freed.
*
* @param m
* The indirect attached packet mbuf.
*/
static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
{
+ struct rte_mbuf *md = rte_mbuf_from_indirect(m);
struct rte_mempool *mp = m->pool;
uint32_t mbuf_size, buf_len, priv_size;
@@ -1482,6 +1487,10 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
m->data_len = 0;
m->ol_flags = 0;
+
+ if (rte_mbuf_refcnt_update(md, -1) == 0) {
+ __rte_mbuf_raw_free(md);
+ }
}
static inline struct rte_mbuf* __attribute__((always_inline))
@@ -1491,15 +1500,9 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
if (likely(rte_mbuf_refcnt_update(m, -1) == 0)) {
- /* if this is an indirect mbuf, then
-* - detach mbuf
-* - free attached mbuf segment
-*/
+ /* if this is an indirect mbuf, it is detached. */
if (RTE_MBUF_INDIRECT(m)) {
- struct rte_mbuf *md = rte_mbuf_from_indirect(m);
rte_pktmbuf_detach(m);
- if (rte_mbuf_refcnt_update(md, -1) == 0)
- __rte_mbuf_raw_free(md);
}
return m;
}
--
1.9.1
[dpdk-dev] [PATCH v2] mbuf: decrease refcnt when detaching
I think this behavior is not part of the API, it is a bug. I agree that detach() frees the direct mbuf when refcnt becomes 0, Konstantin suggests. It is a right behavior of reference counting. Regards, Hiroyuki 2016-05-18 0:45 GMT+09:00 Ananyev, Konstantin : > > >> -Original Message- >> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] >> Sent: Tuesday, May 17, 2016 3:19 PM >> To: Ananyev, Konstantin >> Cc: dev at dpdk.org; Hiroyuki Mikita; olivier.matz at 6wind.com >> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: decrease refcnt when detaching >> >> 2016-05-17 13:44, Ananyev, Konstantin: >> > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] >> > > 2016-05-17 12:59, Ananyev, Konstantin: >> > > > > > The rte_pktmbuf_detach() function should decrease refcnt on a >> > > > > > direct >> > > > > > buffer. >> > > > > >> > > > > As you have noticed, "whenever the indirect buffer is detached, >> > > > > the reference counter on the direct buffer is decremented." >> > > > > So the current behaviour of rte_pktmbuf_detach() is buggy. >> > > > > Why not fix it without renaming? >> > > > > If you consider this behavioral bug is part of the API, we >> > > > > can fix it in a new function unattach and deprecate detach. >> > > > > But Konstantin, why do you want to keep a restore function? >> > > > > What is the need? >> > > > >> > > > I think it might be a useful functionality in some situations: >> > > > some users can attach/detach to external memory buffers (no mbufs) >> > > > and similar functionality is required. >> > > >> > > Attach to external memory buffer (raw buffer) is not currently supported. >> > > >> > > > Let say right now examples/vhost/main.c has its own >> > > > pktmbuf_detach_zcp() >> > > >> > > You should look at the commit http://dpdk.org/commit/68363d85 >> > > "examples/vhost: remove the non-working zero copy code" >> > > >> > > > which is doing pretty much the same - restore original values, after >> > > > detaching >> > > > mbuf from external (virtio) memory buffer. >> > > > Would be good if we'll use a standard API function here. >> > > >> > > You are welcome to implement mbuf attach to raw buffer. >> > > But it is not a requirement for this fix. >> > >> > Hmm, still not sure why we can't keep an existing function? >> >> Because it does not do what its name (and doc) suggest. >> >> > Obviously it wouldn't cost anything and I still think might be useful. >> >> It costs to overcomplicate API for only a half support. > > I still think it is better to have it then not, but wouldn't insist here. > Konstantin > >> If you need the feature "attach to raw", please implement it completely. >
[dpdk-dev] [PATCH v2] mbuf: decrease refcnt when detaching
The rte_pktmbuf_detach() function should decrease refcnt on a direct
buffer.
Signed-off-by: Hiroyuki Mikita
---
v2:
* introduced a new function rte_pktmbuf_detach2() which decrease refcnt.
* marked rte_pktmbuf_detach() as deprecated.
* added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
* checked refcnt when detaching in unit tests.
* added this issue to release notes.
app/test/test_mbuf.c | 9 +--
doc/guides/rel_notes/release_16_07.rst | 11 -
lib/librte_mbuf/rte_mbuf.h | 43 +++---
3 files changed, 51 insertions(+), 12 deletions(-)
diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 98ff93a..2bf05eb 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -438,6 +438,7 @@ test_attach_from_different_pool(void)
struct rte_mbuf *clone = NULL;
struct rte_mbuf *clone2 = NULL;
char *data, *c_data, *c_data2;
+ uint16_t refcnt;
/* alloc a mbuf */
m = rte_pktmbuf_alloc(pktmbuf_pool);
@@ -508,13 +509,17 @@ test_attach_from_different_pool(void)
GOTO_FAIL("invalid refcnt in m\n");
/* detach the clones */
- rte_pktmbuf_detach(clone);
+ refcnt = rte_pktmbuf_detach2(clone);
if (c_data != rte_pktmbuf_mtod(clone, char *))
GOTO_FAIL("clone was not detached properly\n");
+ if (refcnt != 2 || rte_mbuf_refcnt_read(m) != 2)
+ GOTO_FAIL("invalid refcnt in m\n");
- rte_pktmbuf_detach(clone2);
+ refcnt = rte_pktmbuf_detach2(clone2);
if (c_data2 != rte_pktmbuf_mtod(clone2, char *))
GOTO_FAIL("clone2 was not detached properly\n");
+ if (refcnt != 1 || rte_mbuf_refcnt_read(m) != 1)
+ GOTO_FAIL("invalid refcnt in m\n");
/* free the clones and the initial mbuf */
rte_pktmbuf_free(clone2);
diff --git a/doc/guides/rel_notes/release_16_07.rst
b/doc/guides/rel_notes/release_16_07.rst
index f6d543c..9678c1f 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -77,13 +77,10 @@ Other
Known Issues
-This section should contain new known issues in this release. Sample format:
-
-* **Add title in present tense with full stop.**
-
- Add a short 1-2 sentence description of the known issue in the present
- tense. Add information on any known workarounds.
-
+* The ``rte_pktmbuf_detach()`` function does not decrement the direct
+ mbuf's reference counter. It leads a memory leak of the direct
+ mbuf. The workaround is to explicitly decrement the reference
+ counter or use ``rte_pktmbuf_detach2()``.
API Changes
---
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 529debb..c0a592d 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1408,6 +1408,7 @@ static inline int rte_pktmbuf_alloc_bulk(struct
rte_mempool *pool,
*
* After attachment we refer the mbuf we attached as 'indirect',
* while mbuf we attached to as 'direct'.
+ * The direct mbuf's reference counter is incremented.
* Right now, not supported:
* - attachment for already indirect mbuf (e.g. - mi has to be direct).
* - mbuf we trying to attach (mi) is used by someone else
@@ -1459,15 +1460,50 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf
*mi, struct rte_mbuf *m)
/**
* Detach an indirect packet mbuf.
*
+ * Note: It is deprecated.
+ * The direct mbuf's reference counter is not decremented.
+ *
+ * - restore original mbuf address and length values.
+ * - reset pktmbuf data and data_len to their default values.
+ * All other fields of the given packet mbuf will be left intact.
+ *
+ * @param m
+ * The indirect attached packet mbuf.
+ */
+static inline void __rte_deprecated rte_pktmbuf_detach(struct rte_mbuf *m)
+{
+ struct rte_mempool *mp = m->pool;
+ uint32_t mbuf_size, buf_len, priv_size;
+
+ priv_size = rte_pktmbuf_priv_size(mp);
+ mbuf_size = sizeof(struct rte_mbuf) + priv_size;
+ buf_len = rte_pktmbuf_data_room_size(mp);
+
+ m->priv_size = priv_size;
+ m->buf_addr = (char *)m + mbuf_size;
+ m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mbuf_size;
+ m->buf_len = (uint16_t)buf_len;
+ m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
+ m->data_len = 0;
+ m->ol_flags = 0;
+}
+
+/**
+ * Detach an indirect packet mbuf.
+ *
* - restore original mbuf address and length values.
* - reset pktmbuf data and data_len to their default values.
* All other fields of the given packet mbuf will be left intact.
+ * - decrement the direct mbuf's reference counter.
*
* @param m
* The indirect attached packet mbuf.
+ * @return
+ * The updated value of the direct mbuf's reference counter.
*/
-static inline void rte_pktmbuf_detach(struc
[dpdk-dev] [PATCH] mbuf: decrease refcnt when detaching
Hi all, Thanks for suggestions. I think the Oliver's first option is good. I introduce the new function and will replace rte_pktmbuf_detach() with it in a future release. 2016-05-16 18:13 GMT+09:00 Thomas Monjalon : > 2016-05-16 11:46, Hiroyuki MIKITA: >> Now, the attach operation increases refcnt, but the detach does not decrease >> it. >> I think both operations should affect refcnt or not. >> Which design is intended? >> >> In "6.7. Direct and Indirect Buffers" of Programmer's Guide, >> it is mentioned that "...whenever an indirect buffer is attached to >> the direct buffer, >> the reference counter on the direct buffer is incremented. >> Similarly, whenever the indirect buffer is detached, >> the reference counter on the direct buffer is decremented." > > The doc is the reference. The doxygen comment should explicit every > details of the behaviour. > And the unit tests must validate every parts of the behaviour. > Probably there is a bug which is not (yet) tested. > Please see the function testclone_testupdate_testdetach. Thanks >
[dpdk-dev] [PATCH] mbuf: decrease refcnt when detaching
Hi Konstantin,
Now, the attach operation increases refcnt, but the detach does not decrease it.
I think both operations should affect refcnt or not.
Which design is intended?
In "6.7. Direct and Indirect Buffers" of Programmer's Guide,
it is mentioned that "...whenever an indirect buffer is attached to
the direct buffer,
the reference counter on the direct buffer is incremented.
Similarly, whenever the indirect buffer is detached,
the reference counter on the direct buffer is decremented."
Regards,
Hiroyuki
2016-05-16 9:05 GMT+09:00 Ananyev, Konstantin :
>
>
>> -Original Message-
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Hiroyuki Mikita
>> Sent: Sunday, May 15, 2016 4:51 PM
>> To: olivier.matz at 6wind.com
>> Cc: dev at dpdk.org
>> Subject: [dpdk-dev] [PATCH] mbuf: decrease refcnt when detaching
>>
>> The rte_pktmbuf_detach() function should decrease refcnt on a direct
>> buffer.
>
> Hmm, why is that?
> What's wrong with the current approach?
> Konstantin
>
>>
>> Signed-off-by: Hiroyuki Mikita
>> ---
>> lib/librte_mbuf/rte_mbuf.h | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 529debb..3b117ca 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -1468,9 +1468,11 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf
>> *mi, struct rte_mbuf *m)
>> */
>> static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>> {
>> + struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>> struct rte_mempool *mp = m->pool;
>> uint32_t mbuf_size, buf_len, priv_size;
>>
>> + rte_mbuf_refcnt_update(md, -1);
>> priv_size = rte_pktmbuf_priv_size(mp);
>> mbuf_size = sizeof(struct rte_mbuf) + priv_size;
>> buf_len = rte_pktmbuf_data_room_size(mp);
>> @@ -1498,7 +1500,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>> if (RTE_MBUF_INDIRECT(m)) {
>> struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>> rte_pktmbuf_detach(m);
>> - if (rte_mbuf_refcnt_update(md, -1) == 0)
>> + if (rte_mbuf_refcnt_read(md) == 0)
>> __rte_mbuf_raw_free(md);
>> }
>> return m;
>> --
>> 1.9.1
>
[dpdk-dev] [PATCH] mbuf: decrease refcnt when detaching
The rte_pktmbuf_detach() function should decrease refcnt on a direct
buffer.
Signed-off-by: Hiroyuki Mikita
---
lib/librte_mbuf/rte_mbuf.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 529debb..3b117ca 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1468,9 +1468,11 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf
*mi, struct rte_mbuf *m)
*/
static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
{
+ struct rte_mbuf *md = rte_mbuf_from_indirect(m);
struct rte_mempool *mp = m->pool;
uint32_t mbuf_size, buf_len, priv_size;
+ rte_mbuf_refcnt_update(md, -1);
priv_size = rte_pktmbuf_priv_size(mp);
mbuf_size = sizeof(struct rte_mbuf) + priv_size;
buf_len = rte_pktmbuf_data_room_size(mp);
@@ -1498,7 +1500,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
if (RTE_MBUF_INDIRECT(m)) {
struct rte_mbuf *md = rte_mbuf_from_indirect(m);
rte_pktmbuf_detach(m);
- if (rte_mbuf_refcnt_update(md, -1) == 0)
+ if (rte_mbuf_refcnt_read(md) == 0)
__rte_mbuf_raw_free(md);
}
return m;
--
1.9.1
