[PATCH v1 1/2] app/testpmd: fix flow update
The testpmd command “flow update…“ provides a nullptr as the
context variable.
Thus "flow aged destroy" command can not
execute successfully.
Fix was done with next steps
1. Generate new port flow entry to add/replace action(s).
2. Set age contest if exists.
3. Replace flow in the flow list.
Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..bf50f6adef 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
const struct rte_flow_action *actions, bool is_user_id)
{
struct rte_port *port;
- struct port_flow **flow_list;
+ struct port_flow **flow_list, *uf;
+ struct rte_flow_action_age *age = age_action_get(actions);
if (port_id_is_invalid(port_id, ENABLED_WARN) ||
port_id == (portid_t)RTE_PORT_ALL)
@@ -3897,6 +3898,16 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
flow_list = &flow->next;
continue;
}
+
+ /* Update flow action(s) with new action(s) */
+ uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro,
actions, &error);
+ if (!uf)
+ return port_flow_complain(&error);
+ if (age) {
+ flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
+ age->context = &flow->age_type;
+ }
+
/*
* Poisoning to make sure PMDs update it in case
* of error.
@@ -3913,6 +3924,13 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
printf("Flow rule #%"PRIu64
" updated with new actions\n",
flow->id);
+
+ uf->next = flow->next;
+ uf->id = flow->id;
+ uf->flow = flow->flow;
+ *flow_list = uf;
+
+ free(flow);
return 0;
}
printf("Failed to find flow %"PRIu32"\n", rule_id);
--
2.43.5
[PATCH v1 2/2] app/testpmd: fix flow destroy
Avoid removal of additional flows after requested number of flows has
been already removed.
Issue with removal of multiple flows is internal testpmd bug at
port_flow_destroy(). This function goes through all flows and compares
given flow ‘id’ with them. However in some cases it can advance pointer
with “given ID” and thus remove additional flow.
Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index bf50f6adef..50c4b018c1 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4170,8 +4170,12 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
ctx.pf->rule.attr->ingress ? 'i' : '-',
ctx.pf->rule.attr->egress ? 'e' : '-',
ctx.pf->rule.attr->transfer ? 't' : '-');
+ /* use local copy of id as ctx.pf is freed by
+* port_flow_destroy() during processing
+*/
+ uint64_t flow_id = ctx.pf->id;
if (destroy && !port_flow_destroy(port_id, 1,
- &ctx.pf->id, false))
+ &flow_id, false))
total++;
break;
case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
--
2.43.5
[PATCH v1 0/2] Testpmd flow update/destroy fixes
These patches provide next fixes: 1. The testpmd command “flow update…“ provides a nullptr as the context variable. 2. Avoid removal of additional flows after requested number of flows has been already removed. Danylo Vodopianov (2): app/testpmd: fix flow update app/testpmd: fix flow destroy app/test-pmd/config.c | 26 -- 1 file changed, 24 insertions(+), 2 deletions(-) -- 2.43.5
[PATCH v2 0/2] Testpmd flow update/destroy fixes
These patches provide next fixes: 1. The testpmd command “flow update…“ provides a nullptr as the context variable. 2. Avoid removal of additional flows after requested number of flows has been already removed. v2: 1. Rephase commit messages. 2. Copy user_id to the flow list for flow_update command. 3. Enclose the case's body for flow_destroy command in braces. Danylo Vodopianov (2): app/testpmd: fix flow update app/testpmd: fix aged flow destroy app/test-pmd/config.c | 27 --- 1 file changed, 24 insertions(+), 3 deletions(-) -- 2.43.5
[PATCH v2 1/2] app/testpmd: fix flow update
If actions provided to “flow update…“ command contained an age
action, then testpmd did not update the age action context
accordingly.
Thus "flow aged destroy" command can not
execute successfully.
Fix was done with next steps
1. Generate new port flow entry to add/replace action(s).
2. Set age context if age action is present.
3. Replace flow in the flow list.
Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
Cc: sta...@dpdk.org
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..c831166431 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
const struct rte_flow_action *actions, bool is_user_id)
{
struct rte_port *port;
- struct port_flow **flow_list;
+ struct port_flow **flow_list, *uf;
+ struct rte_flow_action_age *age = age_action_get(actions);
if (port_id_is_invalid(port_id, ENABLED_WARN) ||
port_id == (portid_t)RTE_PORT_ALL)
@@ -3897,6 +3898,16 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
flow_list = &flow->next;
continue;
}
+
+ /* Update flow action(s) with new action(s) */
+ uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro,
actions, &error);
+ if (!uf)
+ return port_flow_complain(&error);
+ if (age) {
+ flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
+ age->context = &flow->age_type;
+ }
+
/*
* Poisoning to make sure PMDs update it in case
* of error.
@@ -3913,6 +3924,14 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
printf("Flow rule #%"PRIu64
" updated with new actions\n",
flow->id);
+
+ uf->next = flow->next;
+ uf->id = flow->id;
+ uf->user_id = flow->user_id;
+ uf->flow = flow->flow;
+ *flow_list = uf;
+
+ free(flow);
return 0;
}
printf("Failed to find flow %"PRIu32"\n", rule_id);
--
2.43.5
[PATCH v2 2/2] app/testpmd: fix aged flow destroy
Avoid removal of additional flows after requested number of flows has
been already removed.
port_flow_destroy() function goes through all flows and compares
given flow ‘id’ with them. However in some cases it can advance pointer
with “given ID” and thus remove additional flow.
port_flow_destroy() function never assumed that rule array can be freed
when it's executing, and port_flow_aged() just violated that assumption.
Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")
Cc: sta...@dpdk.org
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index c831166431..04de2fe59d 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4160,8 +4160,9 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
}
type = (enum age_action_context_type *)contexts[idx];
switch (*type) {
- case ACTION_AGE_CONTEXT_TYPE_FLOW:
+ case ACTION_AGE_CONTEXT_TYPE_FLOW: {
ctx.pf = container_of(type, struct port_flow, age_type);
+ uint64_t flow_id = ctx.pf->id;
printf("%-20s\t%" PRIu64 "\t%" PRIu32 "\t%" PRIu32
"\t%c%c%c\t\n",
"Flow",
@@ -4172,9 +4173,10 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
ctx.pf->rule.attr->egress ? 'e' : '-',
ctx.pf->rule.attr->transfer ? 't' : '-');
if (destroy && !port_flow_destroy(port_id, 1,
- &ctx.pf->id, false))
+ &flow_id, false))
total++;
break;
+ }
case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
ctx.pia = container_of(type,
struct port_indirect_action, age_type);
--
2.43.5
[PATCH v3 1/2] app/testpmd: fix flow update
If actions provided to “flow update…“ command contained an age
action, then testpmd did not update the age action context
accordingly.
Thus "flow aged destroy" command can not
execute successfully.
Fix was done with next steps
1. Generate new port flow entry to add/replace action(s).
2. Set age context if age action is present.
3. Replace flow in the flow list.
Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
Cc: sta...@dpdk.org
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..c831166431 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
const struct rte_flow_action *actions, bool is_user_id)
{
struct rte_port *port;
- struct port_flow **flow_list;
+ struct port_flow **flow_list, *uf;
+ struct rte_flow_action_age *age = age_action_get(actions);
if (port_id_is_invalid(port_id, ENABLED_WARN) ||
port_id == (portid_t)RTE_PORT_ALL)
@@ -3897,6 +3898,16 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
flow_list = &flow->next;
continue;
}
+
+ /* Update flow action(s) with new action(s) */
+ uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro,
actions, &error);
+ if (!uf)
+ return port_flow_complain(&error);
+ if (age) {
+ flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
+ age->context = &flow->age_type;
+ }
+
/*
* Poisoning to make sure PMDs update it in case
* of error.
@@ -3913,6 +3924,14 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
printf("Flow rule #%"PRIu64
" updated with new actions\n",
flow->id);
+
+ uf->next = flow->next;
+ uf->id = flow->id;
+ uf->user_id = flow->user_id;
+ uf->flow = flow->flow;
+ *flow_list = uf;
+
+ free(flow);
return 0;
}
printf("Failed to find flow %"PRIu32"\n", rule_id);
--
2.43.5
[PATCH v2 0/2] Testpmd flow update/destroy fixes
These patches provide next fixes: 1. The testpmd command “flow update…“ provides a nullptr as the context variable. 2. Avoid removal of additional flows after requested number of flows has been already removed. v2: 1. Rephase commit messages. 2. Copy user_id to the flow list for flow_update command. 3. Enclose the case's body for flow_destroy command in braces. Danylo Vodopianov (2): app/testpmd: fix flow update app/testpmd: fix aged flow destroy app/test-pmd/config.c | 27 --- 1 file changed, 24 insertions(+), 3 deletions(-) -- 2.43.5
[PATCH v2 0/2] Testpmd flow update/destroy fixes
These patches provide next fixes: 1. The testpmd command “flow update…“ provides a nullptr as the context variable. 2. Avoid removal of additional flows after requested number of flows has been already removed. v2: 1. Rephase commit messages. 2. Copy user_id to the flow list for flow_update command. 3. Enclose the case's body for flow_destroy command in braces. Danylo Vodopianov (2): app/testpmd: fix flow update app/testpmd: fix aged flow destroy app/test-pmd/config.c | 27 --- 1 file changed, 24 insertions(+), 3 deletions(-) -- 2.43.5
[PATCH v3 2/2] app/testpmd: fix aged flow destroy
port_flow_destroy() function never assumed that rule array can be freed
when it's executing, and port_flow_aged() just violated that assumption.
In case of flow async create failure, it tries to do a cleanup, but it
wrongly removes a 1st flow (with id 0). pf->id is not set at this
moment and it always is 0, thus 1st flow is removed. A local copy of
flow->id must be used to call of port_flow_destroy() to avoid access
and processing of flow->id after the flow is removed.
Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")
Cc: sta...@dpdk.org
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index c831166431..28d45568ac 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4160,8 +4160,10 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
}
type = (enum age_action_context_type *)contexts[idx];
switch (*type) {
- case ACTION_AGE_CONTEXT_TYPE_FLOW:
+ case ACTION_AGE_CONTEXT_TYPE_FLOW: {
+ uint64_t flow_id;
ctx.pf = container_of(type, struct port_flow, age_type);
+ flow_id = ctx.pf->id;
printf("%-20s\t%" PRIu64 "\t%" PRIu32 "\t%" PRIu32
"\t%c%c%c\t\n",
"Flow",
@@ -4172,9 +4174,10 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
ctx.pf->rule.attr->egress ? 'e' : '-',
ctx.pf->rule.attr->transfer ? 't' : '-');
if (destroy && !port_flow_destroy(port_id, 1,
- &ctx.pf->id, false))
+ &flow_id, false))
total++;
break;
+ }
case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
ctx.pia = container_of(type,
struct port_indirect_action, age_type);
--
2.43.5
[PATCH v3 0/2] Testpmd flow update/destroy fixes
These patches provide next fixes: 1. The testpmd command “flow update…“ provides a nullptr as the context variable. 2. Avoid removal of additional flows after requested number of flows has been already removed. v2: 1. Rephase commit messages. 2. Copy user_id to the flow list for flow_update command. 3. Enclose the case's body for flow_destroy command in braces. v3: 1. Update commit message 2. Aligned variable declaration code style Danylo Vodopianov (2): app/testpmd: fix flow update app/testpmd: fix aged flow destroy app/test-pmd/config.c | 28 +--- 1 file changed, 25 insertions(+), 3 deletions(-) -- 2.43.5
[PATCH v2 2/2] app/testpmd: fix aged flow destroy
Avoid removal of additional flows after requested number of flows has
been already removed.
port_flow_destroy() function goes through all flows and compares
given flow ‘id’ with them. However in some cases it can advance pointer
with “given ID” and thus remove additional flow.
port_flow_destroy() function never assumed that rule array can be freed
when it's executing, and port_flow_aged() just violated that assumption.
Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")
Cc: sta...@dpdk.org
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index c831166431..04de2fe59d 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4160,8 +4160,9 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
}
type = (enum age_action_context_type *)contexts[idx];
switch (*type) {
- case ACTION_AGE_CONTEXT_TYPE_FLOW:
+ case ACTION_AGE_CONTEXT_TYPE_FLOW: {
ctx.pf = container_of(type, struct port_flow, age_type);
+ uint64_t flow_id = ctx.pf->id;
printf("%-20s\t%" PRIu64 "\t%" PRIu32 "\t%" PRIu32
"\t%c%c%c\t\n",
"Flow",
@@ -4172,9 +4173,10 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
ctx.pf->rule.attr->egress ? 'e' : '-',
ctx.pf->rule.attr->transfer ? 't' : '-');
if (destroy && !port_flow_destroy(port_id, 1,
- &ctx.pf->id, false))
+ &flow_id, false))
total++;
break;
+ }
case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
ctx.pia = container_of(type,
struct port_indirect_action, age_type);
--
2.43.5
[PATCH v2 1/2] app/testpmd: fix flow update
If actions provided to “flow update…“ command contained an age
action, then testpmd did not update the age action context
accordingly.
Thus "flow aged destroy" command can not
execute successfully.
Fix was done with next steps
1. Generate new port flow entry to add/replace action(s).
2. Set age context if age action is present.
3. Replace flow in the flow list.
Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
Cc: sta...@dpdk.org
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..c831166431 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
const struct rte_flow_action *actions, bool is_user_id)
{
struct rte_port *port;
- struct port_flow **flow_list;
+ struct port_flow **flow_list, *uf;
+ struct rte_flow_action_age *age = age_action_get(actions);
if (port_id_is_invalid(port_id, ENABLED_WARN) ||
port_id == (portid_t)RTE_PORT_ALL)
@@ -3897,6 +3898,16 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
flow_list = &flow->next;
continue;
}
+
+ /* Update flow action(s) with new action(s) */
+ uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro,
actions, &error);
+ if (!uf)
+ return port_flow_complain(&error);
+ if (age) {
+ flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
+ age->context = &flow->age_type;
+ }
+
/*
* Poisoning to make sure PMDs update it in case
* of error.
@@ -3913,6 +3924,14 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
printf("Flow rule #%"PRIu64
" updated with new actions\n",
flow->id);
+
+ uf->next = flow->next;
+ uf->id = flow->id;
+ uf->user_id = flow->user_id;
+ uf->flow = flow->flow;
+ *flow_list = uf;
+
+ free(flow);
return 0;
}
printf("Failed to find flow %"PRIu32"\n", rule_id);
--
2.43.5
[PATCH v2 1/2] app/testpmd: fix flow update
If actions provided to “flow update…“ command contained an age
action, then testpmd did not update the age action context
accordingly.
Thus "flow aged destroy" command can not
execute successfully.
Fix was done with next steps
1. Generate new port flow entry to add/replace action(s).
2. Set age context if age action is present.
3. Replace flow in the flow list.
Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
Cc: sta...@dpdk.org
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..c831166431 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
const struct rte_flow_action *actions, bool is_user_id)
{
struct rte_port *port;
- struct port_flow **flow_list;
+ struct port_flow **flow_list, *uf;
+ struct rte_flow_action_age *age = age_action_get(actions);
if (port_id_is_invalid(port_id, ENABLED_WARN) ||
port_id == (portid_t)RTE_PORT_ALL)
@@ -3897,6 +3898,16 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
flow_list = &flow->next;
continue;
}
+
+ /* Update flow action(s) with new action(s) */
+ uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro,
actions, &error);
+ if (!uf)
+ return port_flow_complain(&error);
+ if (age) {
+ flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
+ age->context = &flow->age_type;
+ }
+
/*
* Poisoning to make sure PMDs update it in case
* of error.
@@ -3913,6 +3924,14 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
printf("Flow rule #%"PRIu64
" updated with new actions\n",
flow->id);
+
+ uf->next = flow->next;
+ uf->id = flow->id;
+ uf->user_id = flow->user_id;
+ uf->flow = flow->flow;
+ *flow_list = uf;
+
+ free(flow);
return 0;
}
printf("Failed to find flow %"PRIu32"\n", rule_id);
--
2.43.5
[PATCH v2 2/2] app/testpmd: fix aged flow destroy
Avoid removal of additional flows after requested number of flows has
been already removed.
port_flow_destroy() function goes through all flows and compares
given flow ‘id’ with them. However in some cases it can advance pointer
with “given ID” and thus remove additional flow.
port_flow_destroy() function never assumed that rule array can be freed
when it's executing, and port_flow_aged() just violated that assumption.
Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")
Cc: sta...@dpdk.org
Signed-off-by: Danylo Vodopianov
---
app/test-pmd/config.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index c831166431..04de2fe59d 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4160,8 +4160,9 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
}
type = (enum age_action_context_type *)contexts[idx];
switch (*type) {
- case ACTION_AGE_CONTEXT_TYPE_FLOW:
+ case ACTION_AGE_CONTEXT_TYPE_FLOW: {
ctx.pf = container_of(type, struct port_flow, age_type);
+ uint64_t flow_id = ctx.pf->id;
printf("%-20s\t%" PRIu64 "\t%" PRIu32 "\t%" PRIu32
"\t%c%c%c\t\n",
"Flow",
@@ -4172,9 +4173,10 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
ctx.pf->rule.attr->egress ? 'e' : '-',
ctx.pf->rule.attr->transfer ? 't' : '-');
if (destroy && !port_flow_destroy(port_id, 1,
- &ctx.pf->id, false))
+ &flow_id, false))
total++;
break;
+ }
case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
ctx.pia = container_of(type,
struct port_indirect_action, age_type);
--
2.43.5
RE: [PATCH v4 1/1] vhost: handle virtqueue locking for memory hotplug
Hello, Maxime
Thank you for your review.
If I understand correctly, you propose modifying the VHOST_USER_ASSERT_LOCK()
macro so that a VHOST_USER_SET_MEM_TABLE request does not trigger an assertion.
However, I believe such modification would not be appropriate, as it would
revert the logic introduced in commit 5e8fcc60b59d ("vhost: enhance virtqueue
access lock asserts"). With this approach, we would be performing memory
hotplug without queue locking, which could lead to unintended consequences.
Regarding VDPA device regression. We faced with this issue when we request the
number of lcores that the default amount of memory on the socket cannot handle
it.
So, regression occurred during the startup part, during device configuration
when it creates pkt mbuf pool.
Let me know your thoughts regarding this.
От: Maxime Coquelin
Отправлено: 3 июня 2025 г. 15:30
Кому: Danylo Vodopianov ; tho...@monjalon.net
; aman.deep.si...@intel.com ;
yuying.zh...@intel.com ; or...@nvidia.com
; mcoqu...@redhat.com ; Christian Koue
Muf ; ma...@mellanox.com ;
david.march...@redhat.com ; Mykola Kostenok
; Serhii Iliushyk
Копия: step...@networkplumber.org ; dev@dpdk.org
; Chenbo Xia
Тема: Re: [PATCH v4 1/1] vhost: handle virtqueue locking for memory hotplug
Hello Danylo,
On 6/2/25 10:50 AM, Danylo Vodopianov wrote:
> For vDPA devices, virtqueues are not locked once the device has been
> configured. In the
> commit 5e8fcc60b59d ("vhost: enhance virtqueue access lock asserts"),
> the asserts were enhanced to trigger rte_panic functionality, preventing
> access to virtqueues without locking. However, this change introduced
> an issue where the memory hotplug mechanism, added in the
> commit 127f9c6f7b78 ("vhost: handle memory hotplug with vDPA devices"),
> no longer works. Since it expects for all queues are locked.
>
> During the initialization of a vDPA device, the driver sets the
> VIRTIO_DEV_VDPA_CONFIGURED flag, which prevents the
> vhost_user_lock_all_queue_pairs function from locking the
> virtqueues. This leads to the error: the VIRTIO_DEV_VDPA_CONFIGURED
> flag allows the use of the hotplug mechanism, but it fails
> because the virtqueues are not locked, while it expects to be locked
> for VHOST_USER_SET_MEM_TABLE in the table VHOST_MESSAGE_HANDLERS.
>
> This patch addresses the issue by enhancing the conditional statement
> to include a new condition. Specifically, when the device receives the
> VHOST_USER_SET_MEM_TABLE request, the virtqueues are locked to update
> the basic configurations and hotplug the guest memory.
>
> This fix does not impact access lock when vDPA driver is configured
> for other unnecessary message handlers.
>
> Manual memory configuring with "--socket-mem" option allows to avoid
> hotplug mechanism using.
s/using/use/
It needs a fixes tag, and sta...@dpdk.org should be cc'ed, so that it
gets backported to LTS branches.
>
> Signed-off-by: Danylo Vodopianov
> ---
> lib/vhost/vhost_user.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index ec950acf97..16d03e1033 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -3178,7 +3178,13 @@ vhost_user_msg_handler(int vid, int fd)
> * would cause a dead lock.
> */
>if (msg_handler != NULL && msg_handler->lock_all_qps) {
> - if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
> + /* Lock all queue pairs if the device is not configured for
> vDPA,
> + * or if it is configured for vDPA but the request is
> VHOST_USER_SET_MEM_TABLE.
> + * This ensures proper queue locking for memory table updates
> and guest
> + * memory hotplug.
> + */
> + if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
> + request == VHOST_USER_SET_MEM_TABLE) {
It looks like a workaround, and I'm afraid it could cause regression
with some vDPA devices, or that it would not be enough and we would have
to add other requests as exception.
Wouldn't it better to modify VHOST_USER_ASSERT_LOCK() so that it takes
into account the VIRTIO_DEV_VDPA_CONFIGURED flag?
Thanks,
Maxime
>vhost_user_lock_all_queue_pairs(dev);
>unlock_required = 1;
>}
[PATCH v2 1/1] vhost: handle virtqueue locking for memory hotplug
For vDPA devices, virtqueues are not locked once the device has been
configured. In the commit
5e8fcc60b59d ("vhost: enhance virtqueue access lock asserts"),
the asserts were enhanced to trigger rte_panic functionality, preventing
access to virtqueues without locking. However, this change introduced
an issue where the memory hotplug mechanism, added in the commit
127f9c6f7b78 ("vhost: handle memory hotplug with vDPA devices"),
no longer works. Since it expects for all queues are locked.
During the initialization of a vDPA device, the driver sets the
VIRTIO_DEV_VDPA_CONFIGURED flag, which prevents the
vhost_user_lock_all_queue_pairs function from locking the
virtqueues. This leads to the error: the VIRTIO_DEV_VDPA_CONFIGURED
flag allows the use of the hotplug mechanism, but it fails
because the virtqueues are not locked, while it expects to be locked
for VHOST_USER_SET_MEM_TABLE in the table VHOST_MESSAGE_HANDLERS.
This patch addresses the issue by enhancing the conditional statement
to include a new condition. Specifically, when the device receives the
VHOST_USER_SET_MEM_TABLE request, the virtqueues are locked to update
the basic configurations and hotplug the guest memory.
This fix does not impact access lock when vDPA driver is configured
for other unnecessary message handlers.
Manual memory configuring with "--socket-mem" option allows to avoid
hotplug mechanism using.
Signed-off-by: Danylo Vodopianov
---
lib/vhost/vhost_user.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index ec950acf97..16d03e1033 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -3178,7 +3178,13 @@ vhost_user_msg_handler(int vid, int fd)
* would cause a dead lock.
*/
if (msg_handler != NULL && msg_handler->lock_all_qps) {
- if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
+ /* Lock all queue pairs if the device is not configured for
vDPA,
+* or if it is configured for vDPA but the request is
VHOST_USER_SET_MEM_TABLE.
+* This ensures proper queue locking for memory table updates
and guest
+* memory hotplug.
+*/
+ if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
+ request == VHOST_USER_SET_MEM_TABLE) {
vhost_user_lock_all_queue_pairs(dev);
unlock_required = 1;
}
--
2.47.1
[PATCH v3 0/1] Memory hotplug fix
Hi everyone.
We detected an issue with the memory hotplug feature and virtio devices while
working with vitrio
in DPDK.In general, the issue is around the user request
VHOST_USER_SET_MEM_TABLE and
locking queues for this request
(https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n1512)
When the vhost_user receives the request VHOST_USER_SET_MEM_TABLE, it locks
queues only
when the flag lock_all_qps is true(it is always true), AND the flag
VIRTIO_DEV_VDPA_CONFIGURED is not set.
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n3181
In the case of a memory hot plug, the flag VIRTIO_DEV_VDPA_CONFIGURED is always
set,
and due to this, the queues are never locked while they are expected to be
locked.
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n1512.
The quick solution is to add the check for request type and do lock queues
always
if we have request VHOST_USER_SET_MEM_TABLE. Something like this, in general
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n3179
if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
request == VHOST_USER_SET_MEM_TABLE) {
...
Danylo Vodopianov (1):
vhost: handle virtqueue locking for memory hotplug
lib/vhost/vhost_user.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
--
2.47.1
[PATCH v4 0/1] Memory hotplug fix
Hi everyone.
We detected an issue with the memory hotplug feature and virtio devices while
working with vitrio
in DPDK.In general, the issue is around the user request
VHOST_USER_SET_MEM_TABLE and
locking queues for this request
(https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n1512)
When the vhost_user receives the request VHOST_USER_SET_MEM_TABLE, it locks
queues only
when the flag lock_all_qps is true(it is always true), AND the flag
VIRTIO_DEV_VDPA_CONFIGURED is not set.
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n3181
In the case of a memory hot plug, the flag VIRTIO_DEV_VDPA_CONFIGURED is always
set,
and due to this, the queues are never locked while they are expected to be
locked.
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n1512.
The quick solution is to add the check for request type and do lock queues
always
if we have request VHOST_USER_SET_MEM_TABLE. Something like this, in general
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n3179
if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
request == VHOST_USER_SET_MEM_TABLE) {
...
Danylo Vodopianov (1):
vhost: handle virtqueue locking for memory hotplug
lib/vhost/vhost_user.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
--
2.47.1
[PATCH v3 1/1] vhost: handle virtqueue locking for memory hotplug
For vDPA devices, virtqueues are not locked once the device has been
configured. In the
commit 5e8fcc60b59d ("vhost: enhance virtqueue access lock asserts"),
the asserts were enhanced to trigger rte_panic functionality, preventing
access to virtqueues without locking. However, this change introduced
an issue where the memory hotplug mechanism, added in the commit
127f9c6f7b78 ("vhost: handle memory hotplug with vDPA devices"),
no longer works. Since it expects for all queues are locked.
During the initialization of a vDPA device, the driver sets the
VIRTIO_DEV_VDPA_CONFIGURED flag, which prevents the
vhost_user_lock_all_queue_pairs function from locking the
virtqueues. This leads to the error: the VIRTIO_DEV_VDPA_CONFIGURED
flag allows the use of the hotplug mechanism, but it fails
because the virtqueues are not locked, while it expects to be locked
for VHOST_USER_SET_MEM_TABLE in the table VHOST_MESSAGE_HANDLERS.
This patch addresses the issue by enhancing the conditional statement
to include a new condition. Specifically, when the device receives the
VHOST_USER_SET_MEM_TABLE request, the virtqueues are locked to update
the basic configurations and hotplug the guest memory.
This fix does not impact access lock when vDPA driver is configured
for other unnecessary message handlers.
Manual memory configuring with "--socket-mem" option allows to avoid
hotplug mechanism using.
Signed-off-by: Danylo Vodopianov
---
lib/vhost/vhost_user.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index ec950acf97..16d03e1033 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -3178,7 +3178,13 @@ vhost_user_msg_handler(int vid, int fd)
* would cause a dead lock.
*/
if (msg_handler != NULL && msg_handler->lock_all_qps) {
- if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
+ /* Lock all queue pairs if the device is not configured for
vDPA,
+* or if it is configured for vDPA but the request is
VHOST_USER_SET_MEM_TABLE.
+* This ensures proper queue locking for memory table updates
and guest
+* memory hotplug.
+*/
+ if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
+ request == VHOST_USER_SET_MEM_TABLE) {
vhost_user_lock_all_queue_pairs(dev);
unlock_required = 1;
}
--
2.47.1
[PATCH v4 1/1] vhost: handle virtqueue locking for memory hotplug
For vDPA devices, virtqueues are not locked once the device has been
configured. In the
commit 5e8fcc60b59d ("vhost: enhance virtqueue access lock asserts"),
the asserts were enhanced to trigger rte_panic functionality, preventing
access to virtqueues without locking. However, this change introduced
an issue where the memory hotplug mechanism, added in the
commit 127f9c6f7b78 ("vhost: handle memory hotplug with vDPA devices"),
no longer works. Since it expects for all queues are locked.
During the initialization of a vDPA device, the driver sets the
VIRTIO_DEV_VDPA_CONFIGURED flag, which prevents the
vhost_user_lock_all_queue_pairs function from locking the
virtqueues. This leads to the error: the VIRTIO_DEV_VDPA_CONFIGURED
flag allows the use of the hotplug mechanism, but it fails
because the virtqueues are not locked, while it expects to be locked
for VHOST_USER_SET_MEM_TABLE in the table VHOST_MESSAGE_HANDLERS.
This patch addresses the issue by enhancing the conditional statement
to include a new condition. Specifically, when the device receives the
VHOST_USER_SET_MEM_TABLE request, the virtqueues are locked to update
the basic configurations and hotplug the guest memory.
This fix does not impact access lock when vDPA driver is configured
for other unnecessary message handlers.
Manual memory configuring with "--socket-mem" option allows to avoid
hotplug mechanism using.
Signed-off-by: Danylo Vodopianov
---
lib/vhost/vhost_user.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index ec950acf97..16d03e1033 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -3178,7 +3178,13 @@ vhost_user_msg_handler(int vid, int fd)
* would cause a dead lock.
*/
if (msg_handler != NULL && msg_handler->lock_all_qps) {
- if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
+ /* Lock all queue pairs if the device is not configured for
vDPA,
+* or if it is configured for vDPA but the request is
VHOST_USER_SET_MEM_TABLE.
+* This ensures proper queue locking for memory table updates
and guest
+* memory hotplug.
+*/
+ if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
+ request == VHOST_USER_SET_MEM_TABLE) {
vhost_user_lock_all_queue_pairs(dev);
unlock_required = 1;
}
--
2.47.1
[PATCH v1 1/1] vhost: handle virtqueue locking for memory hotplug
For vDPA devices, virtqueues are not locked once the device has been
configured.
In the commit 5e8fcc60b59d
("vhost: enhance virtqueue access lock asserts"),
the asserts were enhanced to trigger rte_panic functionality, preventing
access to virtqueues without locking. However, this change introduced
an issue where the memory hotplug mechanism, added in the commit
127f9c6f7b78 ("vhost: handle memory hotplug with vDPA devices"),
no longer works. Since it expects for all queues are locked.
During the initialization of a vDPA device, the driver sets the
VIRTIO_DEV_VDPA_CONFIGURED flag, which prevents the
vhost_user_lock_all_queue_pairs function from locking the
virtqueues. This leads to the error: the VIRTIO_DEV_VDPA_CONFIGURED
flag allows the use of the hotplug mechanism, but it fails
because the virtqueues are not locked, while it expects to be locked
for VHOST_USER_SET_MEM_TABLE in the table VHOST_MESSAGE_HANDLERS.
This patch addresses the issue by enhancing the conditional statement
to include a new condition. Specifically, when the device receives the
VHOST_USER_SET_MEM_TABLE request, the virtqueues are locked to update
the basic configurations and hotplug the guest memory.
This fix does not impact access lock when vDPA driver is configured
for other unnecessary message handlers.
Manual memory configuring with "--socket-mem" option allows to avoid
hotplug mechanism using.
Signed-off-by: Danylo Vodopianov
---
lib/vhost/vhost_user.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index ec950acf97..16d03e1033 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -3178,7 +3178,13 @@ vhost_user_msg_handler(int vid, int fd)
* would cause a dead lock.
*/
if (msg_handler != NULL && msg_handler->lock_all_qps) {
- if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
+ /* Lock all queue pairs if the device is not configured for
vDPA,
+* or if it is configured for vDPA but the request is
VHOST_USER_SET_MEM_TABLE.
+* This ensures proper queue locking for memory table updates
and guest
+* memory hotplug.
+*/
+ if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
+ request == VHOST_USER_SET_MEM_TABLE) {
vhost_user_lock_all_queue_pairs(dev);
unlock_required = 1;
}
--
2.43.5
[PATCH v1 0/1] Memory hotplug fix
Hi everyone.
We detected an issue with the memory hotplug feature and virtio devices while
working with vitrio
in DPDK.In general, the issue is around the user request
VHOST_USER_SET_MEM_TABLE and
locking queues for this request
(https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n1512)
When the vhost_user receives the request VHOST_USER_SET_MEM_TABLE, it locks
queues only
when the flag lock_all_qps is true(it is always true), AND the flag
VIRTIO_DEV_VDPA_CONFIGURED is not set.
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n3181
In the case of a memory hot plug, the flag VIRTIO_DEV_VDPA_CONFIGURED is always
set,
and due to this, the queues are never locked while they are expected to be
locked.
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n1512.
The quick solution is to add the check for request type and do lock queues
always
if we have request VHOST_USER_SET_MEM_TABLE. Something like this, in general
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n3179
if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
request == VHOST_USER_SET_MEM_TABLE) {
...
Danylo Vodopianov (1):
vhost: handle virtqueue locking for memory hotplug
lib/vhost/vhost_user.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
--
2.43.5
[PATCH v1 0/1] Memory hotplug fix
Hi everyone.
We detected an issue with the memory hotplug feature and virtio devices while
working with vitrio
in DPDK.In general, the issue is around the user request
VHOST_USER_SET_MEM_TABLE and
locking queues for this request
(https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n1512)
When the vhost_user receives the request VHOST_USER_SET_MEM_TABLE, it locks
queues only
when the flag lock_all_qps is true(it is always true), AND the flag
VIRTIO_DEV_VDPA_CONFIGURED is not set.
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n3181
In the case of a memory hot plug, the flag VIRTIO_DEV_VDPA_CONFIGURED is always
set,
and due to this, the queues are never locked while they are expected to be
locked.
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n1512.
The quick solution is to add the check for request type and do lock queues
always
if we have request VHOST_USER_SET_MEM_TABLE. Something like this, in general
https://git.dpdk.org/dpdk-stable/tree/lib/vhost/vhost_user.c#n3179
if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
request == VHOST_USER_SET_MEM_TABLE) {
...
Danylo Vodopianov (1):
vhost: handle virtqueue locking for memory hotplug
lib/vhost/vhost_user.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
--
2.43.5
[PATCH v1 1/1] vhost: handle virtqueue locking for memory hotplug
For vDPA devices, virtqueues are not locked once the device has been
configured.
In the commit 5e8fcc60b59d
("vhost: enhance virtqueue access lock asserts"),
the asserts were enhanced to trigger rte_panic functionality, preventing
access to virtqueues without locking. However, this change introduced
an issue where the memory hotplug mechanism, added in the commit
127f9c6f7b78 ("vhost: handle memory hotplug with vDPA devices"),
no longer works. Since it expects for all queues are locked.
During the initialization of a vDPA device, the driver sets the
VIRTIO_DEV_VDPA_CONFIGURED flag, which prevents the
vhost_user_lock_all_queue_pairs function from locking the
virtqueues. This leads to the error: the VIRTIO_DEV_VDPA_CONFIGURED
flag allows the use of the hotplug mechanism, but it fails
because the virtqueues are not locked, while it expects to be locked
for VHOST_USER_SET_MEM_TABLE in the table VHOST_MESSAGE_HANDLERS.
This patch addresses the issue by enhancing the conditional statement
to include a new condition. Specifically, when the device receives the
VHOST_USER_SET_MEM_TABLE request, the virtqueues are locked to update
the basic configurations and hotplug the guest memory.
This fix does not impact access lock when vDPA driver is configured
for other unnecessary message handlers.
Manual memory configuring with "--socket-mem" option allows to avoid
hotplug mechanism using.
Signed-off-by: Danylo Vodopianov
---
lib/vhost/vhost_user.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index ec950acf97..16d03e1033 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -3178,7 +3178,13 @@ vhost_user_msg_handler(int vid, int fd)
* would cause a dead lock.
*/
if (msg_handler != NULL && msg_handler->lock_all_qps) {
- if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
+ /* Lock all queue pairs if the device is not configured for
vDPA,
+* or if it is configured for vDPA but the request is
VHOST_USER_SET_MEM_TABLE.
+* This ensures proper queue locking for memory table updates
and guest
+* memory hotplug.
+*/
+ if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
+ request == VHOST_USER_SET_MEM_TABLE) {
vhost_user_lock_all_queue_pairs(dev);
unlock_required = 1;
}
--
2.43.5
RE: [PATCH v4 1/1] vhost: handle virtqueue locking for memory hotplug
Hi, Maxime
I understand your point. However we coould have a situation like this, could
resize twice:
VHOST_CONFIG: (/usr/local/var/run/stdvio4) read message VHOST_USER_SET_MEM_TABLE
VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest memory region size: 0x4000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest physical addr: 0x14000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest virtual addr: 0x14000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) host virtual addr: 0x7fde
VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap addr : 0x7fde
VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap size : 0x4000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap align: 0x4000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap off : 0x0
VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest memory region size: 0x4000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest physical addr: 0x11c000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest virtual addr: 0x11c000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) host virtual addr: 0x7fddc000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap addr : 0x7fddc000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap size : 0x4000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap align: 0x4000
VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap off : 0x0
When we set memory region twice. After first iteration
VIRTIO_DEV_VDPA_CONFIGURED flag will be unset here:
https://github.com/DPDK/dpdk/blob/main/lib/vhost/vhost_user.c#L1425 On the
second iteration, this leads to an rte_panic, as queues are accessed without a
lock.
So we should check vhost message id ( VHOST_USER_SET_MEM_TABLE ).
However, extend VHOST_USER_ASSERT_LOCK macros with additional check if we work
with this message VHOST_USER_SET_MEM_TABLE not handle this case, therefore
translate_ring_addresses function calls q_assert_lock directly, without macros
wrapper. In this function is check access_lock vhost_virtqueue and this case
should be handle.
To address the described issue, we need to make the following changes:
1. Extend VHOST_USER_ASSERT_LOCK macro:
* Add a check for the VHOST_USER_SET_MEM_TABLE message ID.
* Skip rte_panic if the message ID matches VHOST_USER_SET_MEM_TABLE.
2. Modify translate_ring_addresses function:
* Extend its signature to include the id parameter (message ID).
* Add logic to skip vq_assert_lock if the message ID matches
VHOST_USER_SET_MEM_TABLE.
1.
Extend VHOST_USER_ASSERT_LOCK Macro:
#define VHOST_USER_ASSERT_LOCK(dev, vq, id) \
do { \
if ((id) == VHOST_USER_SET_MEM_TABLE) \
break; \
if (!(vq)->access_ok) \
rte_panic("Virtqueue access lock not held\n"); \
} while (0
1.
Modify translate_ring_addresses Function:
static int
translate_ring_addresses(struct virtio_net *dev, struct vhost_virtqueue *vq,
uint32_t id)
{
if (id != VHOST_USER_SET_MEM_TABLE)
vq_assert_lock(dev, vq);
// Existing logic for translating ring addresses...
// ...existing code...
return 0;
}
This approach requires extending more functions with conditional checks to
handle cases where queue locking should be ignored when the memory table is
impacted.
Do you have any thoughts about this? Or I should rework my patchset according
to this described solution above ?
От: Maxime Coquelin
Отправлено: 12 июня 2025 г. 14:38
Кому: Danylo Vodopianov ; tho...@monjalon.net
; aman.deep.si...@intel.com ;
yuying.zh...@intel.com ; or...@nvidia.com
; mcoqu...@redhat.com ; Christian Koue
Muf ; ma...@mellanox.com ;
david.march...@redhat.com ; Mykola Kostenok
; Serhii Iliushyk
Копия: step...@networkplumber.org ; dev@dpdk.org
; Chenbo Xia
Тема: Re: [PATCH v4 1/1] vhost: handle virtqueue locking for memory hotplug
Hi Danylo,
On 6/4/25 10:32 AM, Danylo Vodopianov wrote:
> Hello, Maxime
>
> Thank you for your review.
> If I understand correctly, you propose modifying the |
> VHOST_USER_ASSERT_LOCK()| macro so that a |VHOST_USER_SET_MEM_TABLE|
> request does not trigger an assertion.
> However, I believe such modification would not be appropriate, as it
> would revert the logic introduced in commit |5e8fcc60b59d| ("vhost:
> enhance virtqueue access lock asserts"). With this approach, we would be
> performing memory hotplug without queue locking, which could lead to
> unintended consequences.
> Regarding VDPA device regression. We faced with this issue when we
> request the number of lcores that the default amount of memory on the
> socket cannot handle it.
> So, regression occurred during the startup part, during device
> configuration when it creates pkt mbuf pool.
>
> Let me know your thoughts regarding this.
No, my point was to modify VHOST_USER_ASSERT_LOCK() to no trigger an
assertion in cas
