[dpdk-dev] [PATCH v2] doc: clarify adding author to commit message
Clarify to add the author of the commit in CC while contributing
patches to stable. git-send-email will automatically send the
the patch to this email ID. It is manually removed when the patch
is merged.
Signed-off-by: Honnappa Nagarahalli
---
v2: expand commit log to explain how Cc is used (Thomas)
doc/guides/contributing/patches.rst | 1 +
doc/guides/contributing/stable.rst | 1 +
2 files changed, 2 insertions(+)
diff --git a/doc/guides/contributing/patches.rst
b/doc/guides/contributing/patches.rst
index 16b40225f..282c1fec3 100644
--- a/doc/guides/contributing/patches.rst
+++ b/doc/guides/contributing/patches.rst
@@ -329,6 +329,7 @@ In the commit message body the Cc: sta...@dpdk.org should
be inserted as follows
Update the docs, fixing description of some parameter.
Fixes: abcdefgh1234 ("doc: add some parameter")
+ Cc: aut...@example.com
Cc: sta...@dpdk.org
Signed-off-by: Alex Smith
diff --git a/doc/guides/contributing/stable.rst
b/doc/guides/contributing/stable.rst
index 890bbeccc..747d4bc7c 100644
--- a/doc/guides/contributing/stable.rst
+++ b/doc/guides/contributing/stable.rst
@@ -85,6 +85,7 @@ commit message body as follows::
Update the docs, fixing description of some parameter.
Fixes: abcdefgh1234 ("doc: add some parameter")
+ Cc: aut...@example.com
Cc: sta...@dpdk.org
Signed-off-by: Alex Smith
--
2.17.1
Re: [dpdk-dev] [PATCH V1 2/3] mbuf: change free_cb interface to adapt to GSO case
At 2020-07-31 23:15:43, "Olivier Matz" wrote:
>Hi,
>
>On Thu, Jul 30, 2020 at 08:08:59PM +0800, yang_y...@163.com wrote:
>> From: Yi Yang
>>
>> In GSO case, segmented mbufs are attached to original
>> mbuf which can't be freed when it is external. The issue
>> is free_cb doesn't know original mbuf and doesn't free
>> it when refcnt of shinfo is 0.
>>
>> Original mbuf can be freed by rte_pktmbuf_free segmented
>> mbufs or by rte_pktmbuf_free original mbuf. Two kind of
>> cases should have different behaviors. free_cb won't
>> explicitly call rte_pktmbuf_free to free original mbuf
>> if it is freed by rte_pktmbuf_free original mbuf, but it
>> has to call rte_pktmbuf_free to free original mbuf if it
>> is freed by rte_pktmbuf_free segmented mbufs.
>>
>> In order to fix this issue, free_cb interface has to been
>> changed, __rte_pktmbuf_free_extbuf must deliver called
>> mbuf pointer to free_cb, argument opaque can be defined
>> as a custom struct by user, it can includes original mbuf
>> pointer, user-defined free_cb can compare caller mbuf with
>> mbuf in opaque struct, free_cb should free original mbuf
>> if they are not same, this corresponds to rte_pktmbuf_free
>> segmented mbufs case, otherwise, free_cb won't free original
>> mbuf because the caller explicitly called rte_pktmbuf_free
>> to free it.
>>
>> Here is pseduo code to show two kind of cases.
>>
>> case 1. rte_pktmbuf_free segmented mbufs
>>
>> nb_tx = rte_gso_segment(original_mbuf, /* original mbuf */
>> &gso_ctx,
>> /* segmented mbuf */
>> (struct rte_mbuf **)&gso_mbufs,
>> MAX_GSO_MBUFS);
>
>I'm sorry but it is not very clear to me what operations are done by
>rte_gso_segment().
>
>In the current code, I only see calls to rte_pktmbuf_attach(),
>which do not deal with external buffers. Am I missing something?
>
>Are you able to show the issue only with mbuf functions? It would
>be helpful to understand what does not work.
>
>
>Thanks,
>Olivier
>
Oliver, thank you for comment, let me show you why it doesn't work for my use
case. In OVS DPDK, VM uses vhostuserclient to send large packets whose size is
about 64K because we enabled TSO & UFO, these large packets use rte_mbufs
allocated by DPDK virtio_net function
virtio_dev_pktmbuf_alloc() (in file lib/librte_vhost/virtio_net.c. Please refer
to [PATCH V1 3/3], I changed free_cb as below, these packets use the same
allocate function and the same free_cb no matter they are TCP packet or UDP
packets, in case of VXLAN TSO, most NICs can't support inner UDP fragment
offload, so OVS DPDK has to do it by software, for UDP case, the original
rte_mbuf only can be freed by segmented rte_mbufs which are output packets of
rte_gso_segment, i.e. the original rte_mbuf only can freed by free_cb, you can
see, it explicitly called rte_pktmbuf_free(arg->mbuf), the condition statement
"if (caller_m != arg->mbuf)" is true for this case, this has no problem, but
for TCP case, the original mbuf is delivered to rte_eth_tx_burst() but not
segmented rte_mbufs output by rte_gso_segment, PMD driver will call
rte_pktmbuf_free(original_rte_mbuf) but not
rte_pktmbuf_free(segmented_rte_mbufs), the same free_cb will be called, that
means original_rte_mbuf will be freed twice, you know what will happen, this is
just the issue I'm fixing. I bring in caller_m argument, it can help work
around this because caller_m is arg->mbuf and the condition statement "if
(caller_m != arg->mbuf)" is false, you can't fix it without the change this
patch series did.
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index e663fd4..3b69cbb 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -2136,10 +2136,20 @@ uint16_t rte_vhost_poll_enqueue_completed(int vid,
uint16_t queue_id,
return NULL;
}
+struct shinfo_arg {
+ void *buf;
+ struct rte_mbuf *mbuf;
+};
+
static void
-virtio_dev_extbuf_free(struct rte_mbuf * caller_m __rte_unused, void *opaque)
+virtio_dev_extbuf_free(struct rte_mbuf *caller_m, void *opaque)
{
- rte_free(opaque);
+ struct shinfo_arg *arg = (struct shinfo_arg *)opaque;
+
+ rte_free(arg->buf);
+ if (caller_m != arg->mbuf)
+ rte_pktmbuf_free(arg->mbuf);
+ rte_free(arg);
}
static int
@@ -2172,8 +2182,14 @@ uint16_t rte_vhost_poll_enqueue_completed(int vid,
uint16_t queue_id,
/* Initialize shinfo */
if (shinfo) {
+ struct shinfo_arg *arg = (struct shinfo_arg *)
+ rte_malloc(NULL, sizeof(struct
shinfo_arg),
+
RTE_CACHE_LINE_SIZE);
+
+ arg->buf = buf;
+ arg->mbuf = pkt;
Re: [dpdk-dev] [PATCH] librte_metrics: fix memory leak
Hi Gaurav,
Thanks for the fix. Few comments inline. Also, please follow
contributing guidelines [1]. There is a cheat sheet [2] which is very helpful.
[1] https://doc.dpdk.org/guides/contributing/index.html
[2] https://doc.dpdk.org/guides/contributing/cheatsheet.html
> -Original Message-
> From: dev On Behalf Of Gaurav Singh
> Sent: Saturday, August 1, 2020 1:09 AM
> To: dev@dpdk.org
> Cc: Gaurav Singh
> Subject: [dpdk-dev] [PATCH] librte_metrics: fix memory leak
should be
'lib/metrics'
>
>
> Fix memory leak issue
>
Needs "Fixes" tag and Cc to sta...@dpdk.org (check the contribution guidelines)
> Signed-off-by: Gaurav Singh
> ---
> lib/librte_metrics/rte_metrics_telemetry.c | 24 +++---
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_metrics/rte_metrics_telemetry.c
> b/lib/librte_metrics/rte_metrics_telemetry.c
> index 289ebae0b..9fbe59c62 100644
> --- a/lib/librte_metrics/rte_metrics_telemetry.c
> +++ b/lib/librte_metrics/rte_metrics_telemetry.c
> @@ -41,12 +41,19 @@ rte_metrics_tel_reg_port_ethdev_to_metrics(uint16_t
> port_id)
> }
>
> xstats_names = malloc(sizeof(*xstats_names) * num_xstats);
> + if (xstats_names == NULL) {
> + METRICS_LOG_ERR("Failed to malloc memory for
> xstats_names");
> + ret = -ENOMEM;
> + return ret;
You can return -ENOMEM directly without assigning it to 'ret'
> + }
> +
> eth_xstats_names = malloc(sizeof(struct rte_eth_xstat_name)
> * num_xstats);
> - if (eth_xstats_names == NULL || xstats_names == NULL) {
> + if (eth_xstats_names == NULL) {
> METRICS_LOG_ERR("Failed to malloc memory for
> xstats_names");
I think it is worth changing to eth_xstats_names
> ret = -ENOMEM;
> - goto free_xstats;
> + free(xstats_names);
> + return ret;
Same here, return -ENOMEM directly
> }
>
> if (rte_eth_xstats_get_names(port_id,
> @@ -54,7 +61,7 @@ rte_metrics_tel_reg_port_ethdev_to_metrics(uint16_t
> port_id)
> METRICS_LOG_ERR("rte_eth_xstats_get_names(%u) len %d
> failed",
> port_id, num_xstats);
> ret = -EPERM;
> - goto free_xstats;
> + return ret;
Any reason for this change? This change leaks memory.
> }
>
> for (i = 0; i < num_xstats; i++)
> @@ -63,9 +70,6 @@ rte_metrics_tel_reg_port_ethdev_to_metrics(uint16_t
> port_id)
> if (ret < 0)
> METRICS_LOG_ERR("rte_metrics_reg_names failed - metrics
> may already be registered");
>
> -free_xstats:
> - free(eth_xstats_names);
> - free(xstats_names);
Any reason for this change? This is leaking memory in the successful case.
> return ret;
> }
>
> @@ -167,9 +171,15 @@ rte_metrics_tel_format_port(uint32_t pid, json_t
> *ports,
> }
>
> metrics = malloc(sizeof(struct rte_metric_value) * num_metrics);
> + if (metrics == NULL) {
> + METRICS_LOG_ERR("Cannot allocate memory");
> + return -ENOMEM;
> + }
> +
> names = malloc(sizeof(struct rte_metric_name) * num_metrics);
> - if (metrics == NULL || names == NULL) {
> + if (names == NULL) {
> METRICS_LOG_ERR("Cannot allocate memory");
> + free(metrics);
> return -ENOMEM;
> }
>
> --
> 2.17.1
[dpdk-dev] [PATCH] net/netvsc: return the correct chimney index
From: Long Li
The code should look into "slab" to figure out the index returned from
rte_bitmap_scan().
Fixes: cc02518132 ("net/netvsc: split send buffers from Tx descriptors")
Cc: sta...@dpdk.org
Signed-off-by: Long Li
---
drivers/net/netvsc/hn_rxtx.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index 86a4c0d74..6b32c2ca7 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -206,11 +206,13 @@ hn_chim_uninit(struct rte_eth_dev *dev)
static uint32_t hn_chim_alloc(struct hn_data *hv)
{
uint32_t index = NVS_CHIM_IDX_INVALID;
- uint64_t slab;
+ uint64_t slab = 0;
rte_spinlock_lock(&hv->chim_lock);
- if (rte_bitmap_scan(hv->chim_bmap, &index, &slab))
+ if (rte_bitmap_scan(hv->chim_bmap, &index, &slab)) {
+ index += rte_bsf64(slab);
rte_bitmap_clear(hv->chim_bmap, index);
+ }
rte_spinlock_unlock(&hv->chim_lock);
return index;
--
2.25.1
