[PATCH] examples/l2fwd-event: fix missing unlock issue
The function l2fwd_get_free_event_port acquires a lock on
'evt_rsrc->evp.lock' at the beginning. This lock is expected
to be released at the first return statement, when no free
event port is available.
Fixes: 080f57bceca4 ("examples/l2fwd-event: add eventdev main loop")
Cc: sta...@dpdk.org
Signed-off-by: Weiguo Li
---
examples/l2fwd-event/l2fwd_event.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/examples/l2fwd-event/l2fwd_event.c
b/examples/l2fwd-event/l2fwd_event.c
index 4b5a032e35..842fd816e5 100644
--- a/examples/l2fwd-event/l2fwd_event.c
+++ b/examples/l2fwd-event/l2fwd_event.c
@@ -140,6 +140,7 @@ l2fwd_get_free_event_port(struct l2fwd_event_resources
*evt_rsrc)
rte_spinlock_lock(&evt_rsrc->evp.lock);
if (index >= evt_rsrc->evp.nb_ports) {
+ rte_spinlock_unlock(&evt_rsrc->evp.lock);
printf("No free event port is available\n");
return -1;
}
--
2.34.1
[PATCH] examples/l3fwd: fix missing unlock issue
The function l3fwd_get_free_event_port acquires a lock on
'evt_rsrc->evp.lock' at the beginning. This lock is expected
to be released at the first return statement, when no free
event port is available.
Fixes: aaf58cb85b62 ("examples/l3fwd: add event port and queue setup")
Cc: sta...@dpdk.org
Signed-off-by: Weiguo Li
---
examples/l3fwd/l3fwd_event.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/examples/l3fwd/l3fwd_event.c b/examples/l3fwd/l3fwd_event.c
index 20be22c6db..40c69baafb 100644
--- a/examples/l3fwd/l3fwd_event.c
+++ b/examples/l3fwd/l3fwd_event.c
@@ -205,6 +205,7 @@ l3fwd_get_free_event_port(struct l3fwd_event_resources
*evt_rsrc)
rte_spinlock_lock(&evt_rsrc->evp.lock);
if (index >= evt_rsrc->evp.nb_ports) {
+ rte_spinlock_unlock(&evt_rsrc->evp.lock);
printf("No free event port is available\n");
return -1;
}
--
2.34.1
Re: [PATCH v6 1/2] bus/pci: add function to enable/disable PASID
On Sat, Nov 4, 2023 at 4:47 AM Abdullah Sevincer wrote: > > This commit implements an internal api to enable and disable PASID for > a device e.g. device driver event/dlb2. git comment can be reworded when apply. > > For kernels when PASID enabled by default it breaks DLB functionality, > hence disabling PASID is required for DLB to function properly. > > PASID capability is not exposed to users hence offset can not be > retrieved by rte_pci_find_ext_capability() api. Therefore, api > implemented in this commit accepts an offset for PASID with an enable > flag which is used to enable/disable PASID. > > Signed-off-by: Abdullah Sevincer Acked-by: Jerin Jacob > --- > drivers/bus/pci/pci_common.c | 7 +++ > drivers/bus/pci/rte_bus_pci.h | 13 + > drivers/bus/pci/version.map | 1 + > lib/pci/rte_pci.h | 4 > 4 files changed, 25 insertions(+)
[PATCH] net/sfc: fix null dereference in syslog
When ctx->sa is null, sfc_err(ctx->sa, ...) will triger a null
dereference in the macro of sfc_err. Use SFC_GENERIC_LOG(ERR, ...)
to avoid that.
Fixes: 44db08d53be3 ("net/sfc: maintain controller to EFX interface mapping")
Cc: sta...@dpdk.org
Signed-off-by: Weiguo Li
---
drivers/net/sfc/sfc_ethdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 1efe64a36a..6d57b2ba26 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -2070,7 +2070,7 @@ sfc_process_mport_journal_cb(void *data, efx_mport_desc_t
*mport,
struct sfc_mport_journal_ctx *ctx = data;
if (ctx == NULL || ctx->sa == NULL) {
- sfc_err(ctx->sa, "received NULL context or SFC adapter");
+ SFC_GENERIC_LOG(ERR, "received NULL context or SFC adapter");
return EINVAL;
}
--
2.34.1
RE: [EXT] [PATCH] examples/l2fwd-event: fix missing unlock issue
> -Original Message-
> From: Weiguo Li
> Sent: Saturday, November 4, 2023 12:52 PM
> To: Sunil Kumar Kori ; Pavan Nikhilesh Bhagavatula
>
> Cc: dev@dpdk.org; sta...@dpdk.org; Weiguo Li
> Subject: [EXT] [PATCH] examples/l2fwd-event: fix missing unlock issue
>
> External Email
>
> --
> The function l2fwd_get_free_event_port acquires a lock on 'evt_rsrc->evp.lock'
> at the beginning. This lock is expected to be released at the first return
> statement, when no free event port is available.
>
> Fixes: 080f57bceca4 ("examples/l2fwd-event: add eventdev main loop")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Weiguo Li
Please Fix issue found by CI at
http://mails.dpdk.org/archives/test-report/2023-November/501341.html
> ---
> examples/l2fwd-event/l2fwd_event.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/examples/l2fwd-event/l2fwd_event.c b/examples/l2fwd-
> event/l2fwd_event.c
> index 4b5a032e35..842fd816e5 100644
> --- a/examples/l2fwd-event/l2fwd_event.c
> +++ b/examples/l2fwd-event/l2fwd_event.c
> @@ -140,6 +140,7 @@ l2fwd_get_free_event_port(struct
> l2fwd_event_resources *evt_rsrc)
>
> rte_spinlock_lock(&evt_rsrc->evp.lock);
> if (index >= evt_rsrc->evp.nb_ports) {
> + rte_spinlock_unlock(&evt_rsrc->evp.lock);
> printf("No free event port is available\n");
> return -1;
> }
> --
> 2.34.1
Re: [PATCH v6 1/2] bus/pci: add function to enable/disable PASID
04/11/2023 08:32, Jerin Jacob: > On Sat, Nov 4, 2023 at 4:47 AM Abdullah Sevincer > wrote: > > > > This commit implements an internal api to enable and disable PASID for > > a device e.g. device driver event/dlb2. > > git comment can be reworded when apply. What do you mean Jerin? > > For kernels when PASID enabled by default it breaks DLB functionality, > > hence disabling PASID is required for DLB to function properly. > > > > PASID capability is not exposed to users hence offset can not be > > retrieved by rte_pci_find_ext_capability() api. Therefore, api > > implemented in this commit accepts an offset for PASID with an enable > > flag which is used to enable/disable PASID. > > > > Signed-off-by: Abdullah Sevincer > > Acked-by: Jerin Jacob
Re: [PATCH v6 1/2] bus/pci: add function to enable/disable PASID
On Fri, Nov 03, 2023 at 01:29:32PM -0500, Abdullah Sevincer wrote: > This commit implements an internal api to enable and disable PASID for > a device e.g. device driver event/dlb2. > > For kernels when PASID enabled by default it breaks DLB functionality, > hence disabling PASID is required for DLB to function properly. > > PASID capability is not exposed to users hence offset can not be > retrieved by rte_pci_find_ext_capability() api. Therefore, api > implemented in this commit accepts an offset for PASID with an enable > flag which is used to enable/disable PASID. > > Signed-off-by: Abdullah Sevincer > --- > drivers/bus/pci/pci_common.c | 7 +++ > drivers/bus/pci/rte_bus_pci.h | 13 + > drivers/bus/pci/version.map | 1 + > lib/pci/rte_pci.h | 4 > 4 files changed, 25 insertions(+) > > diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c > index 921d957bf6..5aac2406f1 100644 > --- a/drivers/bus/pci/pci_common.c > +++ b/drivers/bus/pci/pci_common.c > @@ -938,6 +938,13 @@ rte_pci_set_bus_master(const struct rte_pci_device *dev, > bool enable) > return 0; > } > > +int > +rte_pci_pasid_ena_dis(const struct rte_pci_device *dev, off_t offset, bool > enable) While I realise we are now at v6 of this patchset, and the name was suggested on v4, seeing it implemented I'm afraid I think rte_pci_pasid_ena_dis is not a great name! I also agree that the pasid_set name was a bit misleading too, leaving us with a naming problem. I have two suggestions: * if we want to keep one function - "rte_pci_pasid_set_state", which makes it clear we are not setting the pasid, but the pasid state. * separate this explicitly into rte_pci_pasid_enable() and rte_pci_pasid_disable() functions. This is the cleanest solution but it doesn't align with some of the other functions in pci lib which set the state. Jerin, any further thoughts? and sorry for late feedback. /Bruce
RE: [dpdk-dev] [PATCH] ring: fix unaligned memory access on aarch32
> -Original Message-
> From: Morten Brørup
> Sent: Friday, November 3, 2023 7:04 PM
> To: Phil Yang ; Honnappa Nagarahalli
> ; Ruifeng Wang
> ; dev@dpdk.org
> Cc: david.march...@redhat.com; olivier.m...@6wind.com; Dharmik Jayesh
> Thakkar ; Gavin Hu
> ; nd ; andrew.rybche...@oktetlabs.ru
> Subject: RE: [dpdk-dev] [PATCH] ring: fix unaligned memory access on aarch32
>
> I have for a long time now wondered why the ring functions for
> enqueue/dequeue of 64-bit objects supports unaligned addresses, and now I
> finally found the patch introducing it.
>
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Phil Yang
> > Sent: Monday, 9 March 2020 18.20
> >
> > The 32-bit arm machine doesn't support unaligned memory access. It
> > will cause a bus error on aarch32 with the custom element size ring.
> >
> > Thread 1 "test" received signal SIGBUS, Bus error.
> > __rte_ring_enqueue_elems_64 (n=1, obj_table=0xf5edfe41, prod_head=0, \
> > r=0xf5edfb80) at /build/dpdk/build/include/rte_ring_elem.h:177
> > 177 ring[idx++] = obj[i++];
>
> Which test is this? Why is it using an unaligned array of 64-bit objects?
> (Notice
> that obj_table=0xf5edfe41.)
Can't recollect which test it is. I am guessing one of the unit test cases. We
might have to reinvestigate, not sure why the obj_table is unaligned.
>
> Nobody in their right mind would use an unaligned array of 64-bit objects. You
> can only create such an array if you force the compiler to prevent automatic
> alignment! And all the functions in your application using this array would
> also
> need to support unaligned addressing of these objects.
>
> This seems extremely exotic, and not something any real application would do!
>
> I would like to revert this patch for performance reasons.
Can you provide more details? Platform, test, how much is the regression?
>
> >
> > Fixes: cc4b218790f6 ("ring: support configurable element size")
> >
> > Signed-off-by: Phil Yang
> > Reviewed-by: Ruifeng Wang
> > Reviewed-by: Honnappa Nagarahalli
> > ---
> > lib/librte_ring/rte_ring_elem.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_ring/rte_ring_elem.h
> > b/lib/librte_ring/rte_ring_elem.h index 3976757..663addc 100644
> > --- a/lib/librte_ring/rte_ring_elem.h
> > +++ b/lib/librte_ring/rte_ring_elem.h
> > @@ -160,7 +160,7 @@ __rte_ring_enqueue_elems_64(struct rte_ring *r,
> > uint32_t prod_head,
> > const uint32_t size = r->size;
> > uint32_t idx = prod_head & r->mask;
> > uint64_t *ring = (uint64_t *)&r[1];
> > - const uint64_t *obj = (const uint64_t *)obj_table;
> > + const unaligned_uint64_t *obj = (const unaligned_uint64_t
> > *)obj_table;
> > if (likely(idx + n < size)) {
> > for (i = 0; i < (n & ~0x3); i += 4, idx += 4) {
> > ring[idx] = obj[i];
> > @@ -294,7 +294,7 @@ __rte_ring_dequeue_elems_64(struct rte_ring *r,
> > uint32_t prod_head,
> > const uint32_t size = r->size;
> > uint32_t idx = prod_head & r->mask;
> > uint64_t *ring = (uint64_t *)&r[1];
> > - uint64_t *obj = (uint64_t *)obj_table;
> > + unaligned_uint64_t *obj = (unaligned_uint64_t *)obj_table;
> > if (likely(idx + n < size)) {
> > for (i = 0; i < (n & ~0x3); i += 4, idx += 4) {
> > obj[i] = ring[idx];
> > --
> > 2.7.4
> >
>
> References:
> https://git.dpdk.org/dpdk/commit/lib/librte_ring/rte_ring_elem.h?id=3ba514
> 78a3ab3132c33effc8b132641233275b36
> https://patchwork.dpdk.org/project/dpdk/patch/1583774395-10233-1-git-
> send-email-phil.y...@arm.com/
RE: [dpdk-dev] [PATCH] ring: fix unaligned memory access on aarch32
> From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@arm.com]
> Sent: Saturday, 4 November 2023 17.32
>
> > From: Morten Brørup
> > Sent: Friday, November 3, 2023 7:04 PM
> >
> > I have for a long time now wondered why the ring functions for
> > enqueue/dequeue of 64-bit objects supports unaligned addresses, and
> now I
> > finally found the patch introducing it.
> >
> > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Phil Yang
> > > Sent: Monday, 9 March 2020 18.20
> > >
> > > The 32-bit arm machine doesn't support unaligned memory access. It
> > > will cause a bus error on aarch32 with the custom element size
> ring.
> > >
> > > Thread 1 "test" received signal SIGBUS, Bus error.
> > > __rte_ring_enqueue_elems_64 (n=1, obj_table=0xf5edfe41,
> prod_head=0, \
> > > r=0xf5edfb80) at /build/dpdk/build/include/rte_ring_elem.h:177
> > > 177 ring[idx++] = obj[i++];
> >
> > Which test is this? Why is it using an unaligned array of 64-bit
> objects? (Notice
> > that obj_table=0xf5edfe41.)
> Can't recollect which test it is. I am guessing one of the unit test
> cases. We might have to reinvestigate, not sure why the obj_table is
> unaligned.
Thank you for picking this up, Honnappa.
>
> >
> > Nobody in their right mind would use an unaligned array of 64-bit
> objects. You
> > can only create such an array if you force the compiler to prevent
> automatic
> > alignment! And all the functions in your application using this array
> would also
> > need to support unaligned addressing of these objects.
> >
> > This seems extremely exotic, and not something any real application
> would do!
> >
> > I would like to revert this patch for performance reasons.
> Can you provide more details? Platform, test, how much is the
> regression?
I haven't seen a regression, but I speculate some performance cost on low-end
CPUs. Maybe it is purely academic.
Maybe not purely academic... I just tested on Godbolt, which shows different
code generated:
uint64_t fa(void *p)
{
return *(uint64_t *)p;
}
uint64_t fu(void *p)
{
return *(unaligned_uint64_t *)p;
}
Generates different output:
fa:
ldrdr0, [r0]
bx lr
fu:
mov r3, r0
ldr r0, [r0] @ unaligned
ldr r1, [r3, #4] @ unaligned
bx lr
>
> >
> > >
> > > Fixes: cc4b218790f6 ("ring: support configurable element size")
> > >
> > > Signed-off-by: Phil Yang
> > > Reviewed-by: Ruifeng Wang
> > > Reviewed-by: Honnappa Nagarahalli
> > > ---
> > > lib/librte_ring/rte_ring_elem.h | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/librte_ring/rte_ring_elem.h
> > > b/lib/librte_ring/rte_ring_elem.h index 3976757..663addc 100644
> > > --- a/lib/librte_ring/rte_ring_elem.h
> > > +++ b/lib/librte_ring/rte_ring_elem.h
> > > @@ -160,7 +160,7 @@ __rte_ring_enqueue_elems_64(struct rte_ring *r,
> > > uint32_t prod_head,
> > > const uint32_t size = r->size;
> > > uint32_t idx = prod_head & r->mask;
> > > uint64_t *ring = (uint64_t *)&r[1];
> > > - const uint64_t *obj = (const uint64_t *)obj_table;
> > > + const unaligned_uint64_t *obj = (const unaligned_uint64_t
> > > *)obj_table;
> > > if (likely(idx + n < size)) {
> > > for (i = 0; i < (n & ~0x3); i += 4, idx += 4) {
> > > ring[idx] = obj[i];
> > > @@ -294,7 +294,7 @@ __rte_ring_dequeue_elems_64(struct rte_ring *r,
> > > uint32_t prod_head,
> > > const uint32_t size = r->size;
> > > uint32_t idx = prod_head & r->mask;
> > > uint64_t *ring = (uint64_t *)&r[1];
> > > - uint64_t *obj = (uint64_t *)obj_table;
> > > + unaligned_uint64_t *obj = (unaligned_uint64_t *)obj_table;
> > > if (likely(idx + n < size)) {
> > > for (i = 0; i < (n & ~0x3); i += 4, idx += 4) {
> > > obj[i] = ring[idx];
> > > --
> > > 2.7.4
> > >
> >
> > References:
> >
> https://git.dpdk.org/dpdk/commit/lib/librte_ring/rte_ring_elem.h?id=3ba
> 514
> > 78a3ab3132c33effc8b132641233275b36
> > https://patchwork.dpdk.org/project/dpdk/patch/1583774395-10233-1-git-
> > send-email-phil.y...@arm.com/
[RFC] mempool: CPU cache aligning mempool driver accesses
I tried a little experiment, which gave a 25 % improvement in mempool
perf tests for long bursts (n_get_bulk=32 n_put_bulk=32 n_keep=512
constant_n=0) on a Xeon E5-2620 v4 based system.
This is the concept:
If all accesses to the mempool driver goes through the mempool cache,
we can ensure that these bulk load/stores are always CPU cache aligned,
by using cache->size when loading/storing to the mempool driver.
Furthermore, it is rumored that most applications use the default
mempool cache size, so if the driver tests for that specific value,
it can use rte_memcpy(src,dst,N) with N known at build time, allowing
optimal performance for copying the array of objects.
Unfortunately, I need to change the flush threshold from 1.5 to 2 to
be able to always use cache->size when loading/storing to the mempool
driver.
What do you think?
PS: If we can't get rid of the mempool cache size threshold factor,
we really need to expose it through public APIs. A job for another day.
Signed-off-by: Morten Brørup
---
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index 7a7a9bf6db..b21033209b 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -48,7 +48,7 @@ static void
mempool_event_callback_invoke(enum rte_mempool_event event,
struct rte_mempool *mp);
-#define CACHE_FLUSHTHRESH_MULTIPLIER 1.5
+#define CACHE_FLUSHTHRESH_MULTIPLIER 2
#define CALC_CACHE_FLUSHTHRESH(c) \
((typeof(c))((c) * CACHE_FLUSHTHRESH_MULTIPLIER))
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index df87cd231e..76efeff59e 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1014,7 +1014,7 @@ typedef void (rte_mempool_ctor_t)(struct rte_mempool *,
void *);
* If cache_size is non-zero, the rte_mempool library will try to
* limit the accesses to the common lockless pool, by maintaining a
* per-lcore object cache. This argument must be lower or equal to
- * RTE_MEMPOOL_CACHE_MAX_SIZE and n / 1.5. It is advised to choose
+ * RTE_MEMPOOL_CACHE_MAX_SIZE and n / 2. It is advised to choose
* cache_size to have "n modulo cache_size == 0": if this is
* not the case, some elements will always stay in the pool and will
* never be used. The access to the per-lcore table is of course
@@ -1373,24 +1373,24 @@ rte_mempool_do_generic_put(struct rte_mempool *mp, void
* const *obj_table,
RTE_MEMPOOL_CACHE_STAT_ADD(cache, put_objs, n);
/* The request itself is too big for the cache */
- if (unlikely(n > cache->flushthresh))
+ if (unlikely(n > cache->size))
goto driver_enqueue_stats_incremented;
/*
* The cache follows the following algorithm:
* 1. If the objects cannot be added to the cache without crossing
-* the flush threshold, flush the cache to the backend.
+* the flush threshold, flush a fixed amount of the cache to the
backend.
* 2. Add the objects to the cache.
*/
if (cache->len + n <= cache->flushthresh) {
cache_objs = &cache->objs[cache->len];
- cache->len += n;
} else {
- cache_objs = &cache->objs[0];
- rte_mempool_ops_enqueue_bulk(mp, cache_objs, cache->len);
- cache->len = n;
+ cache->len -= cache->size;
+ cache_objs = &cache->objs[cache->len];
+ rte_mempool_ops_enqueue_bulk(mp, cache_objs, cache->size);
}
+ cache->len += n;
/* Add the objects to the cache. */
rte_memcpy(cache_objs, obj_table, sizeof(void *) * n);
@@ -1547,13 +1547,13 @@ rte_mempool_do_generic_get(struct rte_mempool *mp, void
**obj_table,
return 0;
}
- /* if dequeue below would overflow mem allocated for cache */
- if (unlikely(remaining > RTE_MEMPOOL_CACHE_MAX_SIZE))
+ /* More remaining than the cache size */
+ if (unlikely(remaining > cache->size))
goto driver_dequeue;
- /* Fill the cache from the backend; fetch size + remaining objects. */
+ /* Fill the cache from the backend; fetch size objects. */
ret = rte_mempool_ops_dequeue_bulk(mp, cache->objs,
- cache->size + remaining);
+ cache->size);
if (unlikely(ret < 0)) {
/*
* We are buffer constrained, and not able to allocate
@@ -1565,11 +1565,11 @@ rte_mempool_do_generic_get(struct rte_mempool *mp, void
**obj_table,
}
/* Satisfy the remaining part of the request from the filled cache. */
- cache_objs = &cache->objs[cache->size + remaining];
+ cache_objs = &cache->objs[cache->size];
for (index = 0; index < remaining; index++)
*obj_table++ = *--cache_objs;
- cache->len = cache->size;
+ cache->len = cache->size - remaining;
RTE_MEMPOOL_CACHE_STAT_ADD(cache, get_su
Re: [PATCH v6 1/2] bus/pci: add function to enable/disable PASID
On Sat, Nov 4, 2023 at 7:31 PM Bruce Richardson wrote: > > On Fri, Nov 03, 2023 at 01:29:32PM -0500, Abdullah Sevincer wrote: > > This commit implements an internal api to enable and disable PASID for > > a device e.g. device driver event/dlb2. > > > > For kernels when PASID enabled by default it breaks DLB functionality, > > hence disabling PASID is required for DLB to function properly. > > > > PASID capability is not exposed to users hence offset can not be > > retrieved by rte_pci_find_ext_capability() api. Therefore, api > > implemented in this commit accepts an offset for PASID with an enable > > flag which is used to enable/disable PASID. > > > > Signed-off-by: Abdullah Sevincer > > --- > > drivers/bus/pci/pci_common.c | 7 +++ > > drivers/bus/pci/rte_bus_pci.h | 13 + > > drivers/bus/pci/version.map | 1 + > > lib/pci/rte_pci.h | 4 > > 4 files changed, 25 insertions(+) > > > > diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c > > index 921d957bf6..5aac2406f1 100644 > > --- a/drivers/bus/pci/pci_common.c > > +++ b/drivers/bus/pci/pci_common.c > > @@ -938,6 +938,13 @@ rte_pci_set_bus_master(const struct rte_pci_device > > *dev, bool enable) > > return 0; > > } > > > > +int > > +rte_pci_pasid_ena_dis(const struct rte_pci_device *dev, off_t offset, bool > > enable) > > While I realise we are now at v6 of this patchset, and the name was > suggested on v4, seeing it implemented I'm afraid I think > rte_pci_pasid_ena_dis is not a great name! I also agree that the pasid_set > name was a bit misleading too, leaving us with a naming problem. > I have two suggestions: > > * if we want to keep one function - "rte_pci_pasid_set_state", which makes > it clear we are not setting the pasid, but the pasid state. > * separate this explicitly into rte_pci_pasid_enable() and > rte_pci_pasid_disable() functions. This is the cleanest solution but it > doesn't align with some of the other functions in pci lib which set the > state. > > Jerin, any further thoughts? and sorry for late feedback. Yes. Above two functions are better than ena_dis(). Other option could be, rte_pci_pasid_ctrl() which is aligned with PCI register name. No strong opinion for the name. Feel free to pick one from above three options. > > /Bruce
Re: [PATCH v6 1/2] bus/pci: add function to enable/disable PASID
On Sat, Nov 4, 2023 at 2:49 PM Thomas Monjalon wrote: > > 04/11/2023 08:32, Jerin Jacob: > > On Sat, Nov 4, 2023 at 4:47 AM Abdullah Sevincer > > wrote: > > > > > > This commit implements an internal api to enable and disable PASID for > > > a device e.g. device driver event/dlb2. > > > > git comment can be reworded when apply. > > What do you mean Jerin? Since I am not applying this patch, I thought you can reword something like following bus/pci: support PASID control Add an internal API to control PASID for a given PCIe device. > > > > > For kernels when PASID enabled by default it breaks DLB functionality, > > > hence disabling PASID is required for DLB to function properly. > > > > > > PASID capability is not exposed to users hence offset can not be > > > retrieved by rte_pci_find_ext_capability() api. Therefore, api > > > implemented in this commit accepts an offset for PASID with an enable > > > flag which is used to enable/disable PASID. > > > > > > Signed-off-by: Abdullah Sevincer > > > > Acked-by: Jerin Jacob > > >
[PATCH v4 1/5] kvargs: add one new process API
The rte_kvargs_process() was used to handle key=value (e.g.
socket_id=0), it also supports to handle only-key (e.g. socket_id).
But many drivers's callback can only handle key=value, it will segment
fault if handles only-key. so the patchset [1] was introduced.
Because the patchset [1] modified too much drivers, therefore:
1) A new API rte_kvargs_process_opt() was introduced, it inherits the
function of rte_kvargs_process() which could handle both key=value and
only-key cases.
2) Constraint the rte_kvargs_process() can only handle key=value cases,
it will return -1 when handle only-key case (that is the matched key's
value is NULL).
This patch also make sure the rte_kvargs_process_opt() and
rte_kvargs_process() API both return -1 when the kvlist parameter is
NULL.
[1]
https://patches.dpdk.org/project/dpdk/patch/20230320092110.37295-1-fengcheng...@huawei.com/
Signed-off-by: Chengwen Feng
---
doc/guides/rel_notes/release_23_11.rst | 13
lib/kvargs/rte_kvargs.c| 43 --
lib/kvargs/rte_kvargs.h| 37 --
lib/kvargs/version.map | 3 ++
4 files changed, 84 insertions(+), 12 deletions(-)
diff --git a/doc/guides/rel_notes/release_23_11.rst
b/doc/guides/rel_notes/release_23_11.rst
index 249e5939e1..ea7b758b20 100644
--- a/doc/guides/rel_notes/release_23_11.rst
+++ b/doc/guides/rel_notes/release_23_11.rst
@@ -137,6 +137,19 @@ New Features
a group's miss actions, which are the actions to be performed on packets
that didn't match any of the flow rules in the group.
+* **Updated kvargs process API.**
+
+ * Introduced rte_kvargs_process_opt() API, which inherits the function
+of rte_kvargs_process() and could handle both key=value and only-key
+cases.
+
+ * Constraint rte_kvargs_process() API can only handle key=value cases,
+it will return -1 when handle only-key case (that is the matched key's
+value is NULL).
+
+ * Make sure rte_kvargs_process_opt() and rte_kvargs_process() API both
+return -1 when the kvlist parameter is NULL.
+
* **Updated Amazon ena (Elastic Network Adapter) net driver.**
* Upgraded ENA HAL to latest version.
diff --git a/lib/kvargs/rte_kvargs.c b/lib/kvargs/rte_kvargs.c
index c77bb82feb..adc47f8898 100644
--- a/lib/kvargs/rte_kvargs.c
+++ b/lib/kvargs/rte_kvargs.c
@@ -167,31 +167,56 @@ rte_kvargs_count(const struct rte_kvargs *kvlist, const
char *key_match)
return ret;
}
-/*
- * For each matching key, call the given handler function.
- */
-int
-rte_kvargs_process(const struct rte_kvargs *kvlist,
- const char *key_match,
- arg_handler_t handler,
- void *opaque_arg)
+static int
+kvargs_process_common(const struct rte_kvargs *kvlist,
+ const char *key_match,
+ arg_handler_t handler,
+ void *opaque_arg,
+ bool support_only_key)
{
const struct rte_kvargs_pair *pair;
unsigned i;
if (kvlist == NULL)
- return 0;
+ return -1;
for (i = 0; i < kvlist->count; i++) {
pair = &kvlist->pairs[i];
if (key_match == NULL || strcmp(pair->key, key_match) == 0) {
+ if (!support_only_key && pair->value == NULL)
+ return -1;
if ((*handler)(pair->key, pair->value, opaque_arg) < 0)
return -1;
}
}
+
return 0;
}
+/*
+ * For each matching key in key/value, call the given handler function.
+ */
+int
+rte_kvargs_process(const struct rte_kvargs *kvlist,
+ const char *key_match,
+ arg_handler_t handler,
+ void *opaque_arg)
+{
+ return kvargs_process_common(kvlist, key_match, handler, opaque_arg,
false);
+}
+
+/*
+ * For each matching key in key/value or only-key, call the given handler
function.
+ */
+int
+rte_kvargs_process_opt(const struct rte_kvargs *kvlist,
+ const char *key_match,
+ arg_handler_t handler,
+ void *opaque_arg)
+{
+ return kvargs_process_common(kvlist, key_match, handler, opaque_arg,
true);
+}
+
/* free the rte_kvargs structure */
void
rte_kvargs_free(struct rte_kvargs *kvlist)
diff --git a/lib/kvargs/rte_kvargs.h b/lib/kvargs/rte_kvargs.h
index 4900b750bc..ad0b609ad7 100644
--- a/lib/kvargs/rte_kvargs.h
+++ b/lib/kvargs/rte_kvargs.h
@@ -172,14 +172,17 @@ const char *rte_kvargs_get_with_value(const struct
rte_kvargs *kvlist,
const char *key, const char *value);
/**
- * Call a handler function for each key/value matching the key
+ * Call a handler function for each key=value matching the key
*
- * For each key/value association that matches the given key, calls the
+ * For each key=value association that matches the given key, cal
[PATCH v4 0/5] fix segment fault when parse args
The rte_kvargs_process() was used to parse key-value (e.g. socket_id=0), it also supports to parse only-key (e.g. socket_id). But many drivers's callback can only handle key-value, it will segment fault if handles only-key. so the patchset [1] was introduced. Because the patchset [1] modified too much drivers, therefore: 1) A new API rte_kvargs_process_opt() was introduced, it inherits the function of rte_kvargs_process() which could parse both key-value and only-key. 2) Constraint the rte_kvargs_process() can only parse key-value. This patchset also include one bugfix for kvargs of mvneta driver. [1] https://patches.dpdk.org/project/dpdk/patch/20230320092110.37295-1-fengcheng...@huawei.com/ Chengwen Feng (5): kvargs: add one new process API net/sfc: use new API to parse kvargs net/tap: use new API to parse kvargs common/nfp: use new API to parse kvargs net/mvneta: fix possible out-of-bounds write --- v4: refine API's define and impl which address Ferruh's comments. add common/nfp change commit. v3: introduce new API instead of modify too many drivers which address Ferruh's comments. doc/guides/rel_notes/release_23_11.rst | 13 drivers/common/nfp/nfp_common_pci.c| 4 +-- drivers/common/sfc_efx/sfc_efx.c | 4 +-- drivers/net/mvneta/mvneta_ethdev.c | 3 ++ drivers/net/sfc/sfc_kvargs.c | 2 +- drivers/net/tap/rte_eth_tap.c | 10 +++--- lib/kvargs/rte_kvargs.c| 43 -- lib/kvargs/rte_kvargs.h| 37 -- lib/kvargs/version.map | 3 ++ 9 files changed, 97 insertions(+), 22 deletions(-) -- 2.17.1
[PATCH v4 5/5] net/mvneta: fix possible out-of-bounds write
The mvneta_ifnames_get() function will save 'iface' value to ifnames,
it will out-of-bounds write if passed many iface pairs (e.g.
'iface=xxx,iface=xxx,...').
Fixes: 4ccc8d770d3b ("net/mvneta: add PMD skeleton")
Cc: sta...@dpdk.org
Signed-off-by: Chengwen Feng
Acked-by: Ferruh Yigit
---
drivers/net/mvneta/mvneta_ethdev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/mvneta/mvneta_ethdev.c
b/drivers/net/mvneta/mvneta_ethdev.c
index daa69e533a..8032a712f4 100644
--- a/drivers/net/mvneta/mvneta_ethdev.c
+++ b/drivers/net/mvneta/mvneta_ethdev.c
@@ -91,6 +91,9 @@ mvneta_ifnames_get(const char *key __rte_unused, const char
*value,
{
struct mvneta_ifnames *ifnames = extra_args;
+ if (ifnames->idx >= NETA_NUM_ETH_PPIO)
+ return -EINVAL;
+
ifnames->names[ifnames->idx++] = value;
return 0;
--
2.17.1
[PATCH v4 2/5] net/sfc: use new API to parse kvargs
The sfc_kvargs_process() and sfc_efx_dev_class_get() function could
handle both key=value and only-key, so they should use
rte_kvargs_process_opt() instead of rte_kvargs_process() to parse.
Signed-off-by: Chengwen Feng
---
drivers/common/sfc_efx/sfc_efx.c | 4 ++--
drivers/net/sfc/sfc_kvargs.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/common/sfc_efx/sfc_efx.c b/drivers/common/sfc_efx/sfc_efx.c
index 2dc5545760..3ebac909f1 100644
--- a/drivers/common/sfc_efx/sfc_efx.c
+++ b/drivers/common/sfc_efx/sfc_efx.c
@@ -52,8 +52,8 @@ sfc_efx_dev_class_get(struct rte_devargs *devargs)
return dev_class;
if (rte_kvargs_count(kvargs, RTE_DEVARGS_KEY_CLASS) != 0) {
- rte_kvargs_process(kvargs, RTE_DEVARGS_KEY_CLASS,
- sfc_efx_kvarg_dev_class_handler, &dev_class);
+ rte_kvargs_process_opt(kvargs, RTE_DEVARGS_KEY_CLASS,
+ sfc_efx_kvarg_dev_class_handler,
&dev_class);
}
rte_kvargs_free(kvargs);
diff --git a/drivers/net/sfc/sfc_kvargs.c b/drivers/net/sfc/sfc_kvargs.c
index 783cb43ae6..24bb896179 100644
--- a/drivers/net/sfc/sfc_kvargs.c
+++ b/drivers/net/sfc/sfc_kvargs.c
@@ -70,7 +70,7 @@ sfc_kvargs_process(struct sfc_adapter *sa, const char
*key_match,
if (sa->kvargs == NULL)
return 0;
- return -rte_kvargs_process(sa->kvargs, key_match, handler, opaque_arg);
+ return -rte_kvargs_process_opt(sa->kvargs, key_match, handler,
opaque_arg);
}
int
--
2.17.1
[PATCH v4 3/5] net/tap: use new API to parse kvargs
Some kvargs could be key=value or only-key, it should use
rte_kvargs_process_opt() instead of rte_kvargs_process() to handle
these kvargs.
Signed-off-by: Chengwen Feng
---
drivers/net/tap/rte_eth_tap.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index b41fa971cb..cdb52cf408 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2292,7 +2292,7 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
kvlist = rte_kvargs_parse(params, valid_arguments);
if (kvlist) {
if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
- ret = rte_kvargs_process(kvlist,
+ ret = rte_kvargs_process_opt(kvlist,
ETH_TAP_IFACE_ARG,
&set_interface_name,
tun_name);
@@ -2496,10 +2496,10 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
kvlist = rte_kvargs_parse(params, valid_arguments);
if (kvlist) {
if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
- ret = rte_kvargs_process(kvlist,
-ETH_TAP_IFACE_ARG,
-&set_interface_name,
-tap_name);
+ ret = rte_kvargs_process_opt(kvlist,
+ETH_TAP_IFACE_ARG,
+
&set_interface_name,
+tap_name);
if (ret == -1)
goto leave;
}
--
2.17.1
[PATCH v4 4/5] common/nfp: use new API to parse kvargs
The nfp_parse_class_options() function could handle both key=value and
only-key, so it should use rte_kvargs_process_opt() instead of
rte_kvargs_process() to parse.
Signed-off-by: Chengwen Feng
---
drivers/common/nfp/nfp_common_pci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/common/nfp/nfp_common_pci.c
b/drivers/common/nfp/nfp_common_pci.c
index 723035d0f7..ac0a363992 100644
--- a/drivers/common/nfp/nfp_common_pci.c
+++ b/drivers/common/nfp/nfp_common_pci.c
@@ -171,8 +171,8 @@ nfp_parse_class_options(const struct rte_devargs *devargs)
return dev_class;
if (rte_kvargs_count(kvargs, RTE_DEVARGS_KEY_CLASS) != 0) {
- rte_kvargs_process(kvargs, RTE_DEVARGS_KEY_CLASS,
- nfp_kvarg_dev_class_handler, &dev_class);
+ rte_kvargs_process_opt(kvargs, RTE_DEVARGS_KEY_CLASS,
+ nfp_kvarg_dev_class_handler, &dev_class);
}
rte_kvargs_free(kvargs);
--
2.17.1
Re: [PATCH v3 0/5] fix segment fault when parse args
Hi Ferruh,
On 2023/11/3 21:41, Ferruh Yigit wrote:
> On 11/3/2023 7:38 AM, Chengwen Feng wrote:
>> The rte_kvargs_process() was used to parse key-value (e.g. socket_id=0),
>> it also supports to parse only-key (e.g. socket_id). But many drivers's
>> callback can only handle key-value, it will segment fault if handles
>> only-key. so the patchset [1] was introduced.
>>
>> Because the patchset [1] modified too much drivers, therefore:
>> 1) A new API rte_kvargs_process_opt() was introduced, it inherits the
>> function of rte_kvargs_process() which could parse both key-value and
>> only-key.
>> 2) Constraint the rte_kvargs_process() can only parse key-value.
>>
>> This patchset also include one bugfix for kvargs of mvneta driver.
>>
>> [1]
>> https://patches.dpdk.org/project/dpdk/patch/20230320092110.37295-1-fengcheng...@huawei.com/
>>
>> Chengwen Feng (5):
>> kvargs: add one new process API
>> net/af_packet: use new API to parse kvargs
>> net/sfc: use new API to parse kvargs
>> net/tap: use new API to parse kvargs
>> net/mvneta: fix possible out-of-bounds write
>>
>
> Hi Chengwen,
>
> I checked the driver code updates above, but it is hard to know if there
> are more missing, each requires investigating one by one.
> Perhaps it can be easier to trace back from your original patch [1] and
> update the ones that doesn't need "value == NULL" check, I assume this
> is what you did.
There are 51 files modified in v2 [1], there will about 80 rte_kvargs_process()
invoke in current git (stats by command [2]).
Exclude kvargs lib and it's test and part comment, there are 30 file was not in
v2:
drivers/baseband/null/bbdev_null.c---already treat
NULL value as a error
drivers/baseband/turbo_sw/bbdev_turbo_software.c ---already treat
NULL value as a error
drivers/bus/ifpga/ifpga_bus.c ---already treat
NULL value as a error
drivers/common/nfp/nfp_common_pci.c ---could handle
NULL value
drivers/common/sfc_efx/sfc_efx.c ---could handle
NULL value
drivers/dma/skeleton/skeleton_dmadev.c---already treat
NULL value as a error
drivers/ml/cnxk/cn10k_ml_dev.c---part treat
NULL value as a error, other segment fault when with NULL value
drivers/ml/cnxk/mvtvm_ml_dev.c---segment fault
when with NULL value
drivers/net/af_packet/rte_eth_af_packet.c ---don't care
about value, suggested don't change in v3's comment
drivers/net/bnxt/bnxt_ethdev.c---already treat
NULL value as a error
drivers/net/bonding/rte_eth_bond_pmd.c---already treat
NULL value as a error
drivers/net/cpfl/cpfl_ethdev.c---segment fault
when with NULL value
drivers/net/failsafe/failsafe_args.c ---already treat
NULL value as a error
drivers/net/hns3/hns3_common.c---already treat
NULL value as a error
drivers/net/ixgbe/ixgbe_ethdev.c ---already treat
NULL value as a error
drivers/net/null/rte_eth_null.c ---already treat
NULL value as a error
drivers/net/softnic/rte_eth_softnic.c ---already treat
NULL value as a error
drivers/net/tap/rte_eth_tap.c ---could handle
NULL value
drivers/net/txgbe/txgbe_ethdev.c ---already treat
NULL value as a error
drivers/net/vhost/rte_eth_vhost.c ---already treat
NULL value as a error
drivers/net/virtio/virtio_ethdev.c---already treat
NULL value as a error
drivers/net/virtio/virtio_pci_ethdev.c---already treat
NULL value as a error
drivers/net/virtio/virtio_user_ethdev.c ---already treat
NULL value as a error
drivers/raw/ifpga/ifpga_rawdev.c ---already treat
NULL value as a error
drivers/raw/skeleton/skeleton_rawdev.c---already treat
NULL value as a error
drivers/vdpa/ifc/ifcvf_vdpa.c ---already treat
NULL value as a error
drivers/vdpa/sfc/sfc_vdpa_filter.c---already treat
NULL value as a error
lib/compressdev/rte_compressdev_pmd.c ---already treat
NULL value as a error
lib/cryptodev/cryptodev_pmd.c ---already treat
NULL value as a error
lib/ethdev/rte_ethdev_telemetry.c ---already treat
NULL value as a error
so we should only process three drivers: common/nfp, sfc, tap, and these are
what v4 doing.
[1]
https://patches.dpdk.org/project/dpdk/patch/20230320092110.37295-1-fengcheng...@huawei.com/
[2] grep -rn "rte_kvargs_process(" | cut -d ":" -f 1 | sort | uniq -c | wc -l
Thanks
C
Re: [PATCH v3 1/5] kvargs: add one new process API
Hi Ferruh,
On 2023/11/3 21:09, Ferruh Yigit wrote:
> On 11/3/2023 7:38 AM, Chengwen Feng wrote:
>> The rte_kvargs_process() was used to parse key-value (e.g. socket_id=0),
>> it also supports to parse only-key (e.g. socket_id). But many drivers's
>> callback can only handle key-value, it will segment fault if handles
>> only-key. so the patchset [1] was introduced.
>>
>> Because the patchset [1] modified too much drivers, therefore:
>> 1) A new API rte_kvargs_process_opt() was introduced, it inherits the
>> function of rte_kvargs_process() which could parse both key-value and
>> only-key.
>> 2) Constraint the rte_kvargs_process() can only parse key-value.
>>
>
> Hi Chengwen,
>
> This works, but behavior change in 'rte_kvargs_process()' can hit some
> exiting users who handles both "key=value" and "key" cases.
>
> Other option is to keep 'rte_kvargs_process()' behavior same but add a
> new API like 'rte_kvargs_process_safe()' that checks "value == NULL"
> case, but this requires more existing code to change and in this option
> existing users doesn't get the benefit of the NULL check by default.
>
>
> Assuming number of the users that use 'rte_kvargs_process()' for
> "key=value" is majority, OK to continue with your change, but please
> document this in the release notes to highlight.
Thanks, already fix in v4
>
>
>
>> [1]
>> https://patches.dpdk.org/project/dpdk/patch/20230320092110.37295-1-fengcheng...@huawei.com/
>>
>> Signed-off-by: Chengwen Feng
>> ---
>> lib/kvargs/rte_kvargs.c | 29 -
>> lib/kvargs/rte_kvargs.h | 28
>> lib/kvargs/version.map | 3 +++
>> 3 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/kvargs/rte_kvargs.c b/lib/kvargs/rte_kvargs.c
>> index c77bb82feb..5ce8664238 100644
>> --- a/lib/kvargs/rte_kvargs.c
>> +++ b/lib/kvargs/rte_kvargs.c
>> @@ -168,7 +168,7 @@ rte_kvargs_count(const struct rte_kvargs *kvlist, const
>> char *key_match)
>> }
>>
>> /*
>> - * For each matching key, call the given handler function.
>> + * For each matching key in key/value, call the given handler function.
>> */
>> int
>> rte_kvargs_process(const struct rte_kvargs *kvlist,
>> @@ -179,6 +179,33 @@ rte_kvargs_process(const struct rte_kvargs *kvlist,
>> const struct rte_kvargs_pair *pair;
>> unsigned i;
>>
>> +if (kvlist == NULL)
>> +return 0;
>> +
>
> I think it should return error here, ignoring arg silently with success
> can cause trouble in the application. If error returned, at least
> application can know argument not provided as it should be (value is
> missing).
fix in v4.
>
>
>> +for (i = 0; i < kvlist->count; i++) {
>> +pair = &kvlist->pairs[i];
>> +if (pair->value == NULL)
>> +continue;
>> +if (key_match == NULL || strcmp(pair->key, key_match) == 0) {
>> +if ((*handler)(pair->key, pair->value, opaque_arg) < 0)
>> +return -1;
>> +}
>> +}
>> +return 0;
>> +}
>> +
>
> 'rte_kvargs_process()' & 'rte_kvargs_process_opt()' implementations are
> very similar, to reduce duplication what about create
> 'rte_kvargs_process_common()' static function and both use this common
> with different parameter?
nice catch, fix in v4.
>
>
>> +/*
>> + * For each matching key in key/value or only-key, call the given handler
>> function.
>> + */
>> +int
>> +rte_kvargs_process_opt(const struct rte_kvargs *kvlist,
>> + const char *key_match,
>> + arg_handler_t handler,
>> + void *opaque_arg)
>> +{
>> +const struct rte_kvargs_pair *pair;
>> +unsigned int i;
>> +
>> if (kvlist == NULL)
>> return 0;
>>
>> diff --git a/lib/kvargs/rte_kvargs.h b/lib/kvargs/rte_kvargs.h
>> index 4900b750bc..522e83f757 100644
>> --- a/lib/kvargs/rte_kvargs.h
>> +++ b/lib/kvargs/rte_kvargs.h
>> @@ -195,6 +195,34 @@ const char *rte_kvargs_get_with_value(const struct
>> rte_kvargs *kvlist,
>> int rte_kvargs_process(const struct rte_kvargs *kvlist,
>> const char *key_match, arg_handler_t handler, void *opaque_arg);
>>
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * Call a handler function for each key/value or only-key matching the key
>> + *
>>
>
> In API documentation, the difference between 'rte_kvargs_process_opt()'
> & 'rte_kvargs_process()' is, 'rte_kvargs_process()' doesn't have "or
> only-key", this is easy to miss.
> Can you please add more not to 'rte_kvargs_process()' saying on
> "only-key" case it returns error?
add one @note in v4.
>
>
>> + * For each key/value or only-key association that matches the given key,
>> calls
>> + * the handler function with the for a given arg_name passing the value on
>> the
>> + * dictionary for that key and a given extra argument.
>> + *
>> + * @param kvlist
>> + * The rte_kvargs structure
Re: [PATCH v3 2/5] net/af_packet: use new API to parse kvargs
Hi Ferruh,
On 2023/11/3 21:11, Ferruh Yigit wrote:
> On 11/3/2023 7:38 AM, Chengwen Feng wrote:
>> This driver don't care about the 'iface' value, it should use
>> rte_kvargs_process_opt() instead of rte_kvargs_process() to parse.
>>
>> Signed-off-by: Chengwen Feng
>> ---
>> drivers/net/af_packet/rte_eth_af_packet.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c
>> b/drivers/net/af_packet/rte_eth_af_packet.c
>> index 397a32db58..54a9f5f746 100644
>> --- a/drivers/net/af_packet/rte_eth_af_packet.c
>> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
>> @@ -1096,8 +1096,8 @@ rte_pmd_af_packet_probe(struct rte_vdev_device *dev)
>> */
>> if (rte_kvargs_count(kvlist, ETH_AF_PACKET_IFACE_ARG) == 1) {
>>
>> -ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
>> - &open_packet_iface, &sockfd);
>> +ret = rte_kvargs_process_opt(kvlist, ETH_AF_PACKET_IFACE_ARG,
>> + &open_packet_iface, &sockfd);
>> if (ret < 0)
>> goto exit;
>> }
>
> lets not update this driver, I think it is using kvargs
> unconventionally, 'iface' requires argument but driver parses it
> directly from kvargs.
>
> We can fix kvargs usage more properly instead of this change, I am
> taking a mental note for this.
got, it was droped from v4.
Thanks
Chengwen
> .
>
Re: [PATCH v3 4/5] net/tap: use new API to parse kvargs
Hi Ferruh,
Thanks for deepin, both fix in v4.
On 2023/11/3 21:34, Ferruh Yigit wrote:
> On 11/3/2023 7:38 AM, Chengwen Feng wrote:
>> This driver could handles both key=value and only-key kvargs, so it
>> should use rte_kvargs_process_opt() instead of rte_kvargs_process() to
>> parse.
>>
>> Signed-off-by: Chengwen Feng
>> ---
>> drivers/net/tap/rte_eth_tap.c | 26 +-
>> 1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index b25a52655f..8b35de0a7a 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -2342,7 +2342,7 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
>> kvlist = rte_kvargs_parse(params, valid_arguments);
>> if (kvlist) {
>> if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
>> -ret = rte_kvargs_process(kvlist,
>> +ret = rte_kvargs_process_opt(kvlist,
>> ETH_TAP_IFACE_ARG,
>> &set_interface_name,
>> tun_name);
>> @@ -2546,28 +2546,28 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>> kvlist = rte_kvargs_parse(params, valid_arguments);
>> if (kvlist) {
>> if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
>> -ret = rte_kvargs_process(kvlist,
>> - ETH_TAP_IFACE_ARG,
>> - &set_interface_name,
>> - tap_name);
>> +ret = rte_kvargs_process_opt(kvlist,
>> + ETH_TAP_IFACE_ARG,
>> +
>> &set_interface_name,
>> + tap_name);
>> if (ret == -1)
>> goto leave;
>> }
>>
>> if (rte_kvargs_count(kvlist, ETH_TAP_REMOTE_ARG) == 1) {
>> -ret = rte_kvargs_process(kvlist,
>> - ETH_TAP_REMOTE_ARG,
>> - &set_remote_iface,
>> - remote_iface);
>> +ret = rte_kvargs_process_opt(kvlist,
>> + ETH_TAP_REMOTE_ARG,
>> + &set_remote_iface,
>> + remote_iface);
>>
>
> As far as I can see, "remote" arg without value is not valid, but driver
> handles this case as if "remote" arg is not provided at all. I think it
> is reasonable to keep using 'rte_kvargs_process()', and fail if 'value'
> is not provided.
>
>
>> if (ret == -1)
>> goto leave;
>> }
>>
>> if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
>> -ret = rte_kvargs_process(kvlist,
>> - ETH_TAP_MAC_ARG,
>> - &set_mac_type,
>> - &user_mac);
>> +ret = rte_kvargs_process_opt(kvlist,
>> + ETH_TAP_MAC_ARG,
>> + &set_mac_type,
>> + &user_mac);
>
> same here, 'rte_kvargs_process()' can be used, there is no point to give
> "mac" keyword without value, that is same as not providing "mac" keyword
> at all, so this can fail to notify user either provide a mac or remove
> the argument.
>
> I think current logic is to handle "value==null" case, otherwise this is
> not a valid usecase.
>
>> if (ret == -1)
>> goto leave;
>> }
>
> .
>
