RE: [PATCH v2] net/ice: add devargs for disabling mac filter
> -Original Message-
> From: Zhang, Qi Z
> Sent: Monday, December 26, 2022 2:15 PM
> To: Zhang, Ke1X ; Zhang, Yuying
> ; dev@dpdk.org
> Subject: RE: [PATCH v2] net/ice: add devargs for disabling mac filter
>
>
>
> > -Original Message-
> > From: Zhang, Ke1X
> > Sent: Wednesday, December 21, 2022 11:58 AM
> > To: Zhang, Qi Z ; Zhang, Yuying
> > ; dev@dpdk.org
> > Cc: Zhang, Ke1X
> > Subject: [PATCH v2] net/ice: add devargs for disabling mac filter
> >
> > From: "ke1x.zhang"
> >
> > This patch adds support to disable mac filter which will be used by
> > ice driver when setting dpdk_devargs config field in the TRex config file.
> >
> > Mac filter is not disabled in default.
> >
> > Signed-off-by: ke1x.zhang
> > ---
> > doc/guides/nics/ice.rst| 11 +++
> > drivers/net/ice/ice_ethdev.c | 13 +
> > drivers/net/ice/ice_ethdev.h | 1 +
> > drivers/net/ice/ice_generic_flow.c | 15 +++
> > 4 files changed, 40 insertions(+)
> >
> > diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst index
> > ce075e067c..18b4d12048 100644
> > --- a/doc/guides/nics/ice.rst
> > +++ b/doc/guides/nics/ice.rst
> > @@ -105,6 +105,17 @@ Runtime Config Options
> >
> > -a 80:00.0,pipeline-mode-support=1
> >
> > +- ``Disable Mac Filter Support`` (default ``0``)
>
> I think better to name the feature as "No default Mac" , as we still support
> add/del MAC filter.
Thanks for your comments, there is a question:
If we still support add/del MAC filter, what should do in ice_add_mac_filter()?
In ice_add_mac_filter(), I don't know the caller, maybe it is a initiliazer to
add defaut mac or
add mac filter command.
>
> > +
> > + Add the feature that support to disable mac filter which will be
> > + used by ice driver when setting dpdk_devargs config field in the TRex
> config file.
> > +
>
> No need to mention TRex here, just describe what's the expected behavior
> when user set this flag.
>
> > + Mac filter is not disabled in default, user can choose to disable
> > + the mac filter by setting ``devargs`` parameter
> > + ``mac-filter-disable``, for example::
> > +
> > +-a 80:00.0,mac-filter-disable=1
> > +
> > - ``Protocol extraction for per queue``
> >
> >Configure the RX queues to do protocol extraction into mbuf for
> > protocol diff --git a/drivers/net/ice/ice_ethdev.c
> > b/drivers/net/ice/ice_ethdev.c index
> > 0bc739daf0..0c9f66eb88 100644
> > --- a/drivers/net/ice/ice_ethdev.c
> > +++ b/drivers/net/ice/ice_ethdev.c
> > @@ -28,6 +28,7 @@
> > /* devargs */
> > #define ICE_SAFE_MODE_SUPPORT_ARG "safe-mode-support"
> > #define ICE_PIPELINE_MODE_SUPPORT_ARG "pipeline-mode-support"
> > +#define ICE_MAC_FILTER_DISABLE "mac-filter-disable"
> > #define ICE_PROTO_XTR_ARG "proto_xtr"
> > #define ICE_FIELD_OFFS_ARG "field_offs"
> > #define ICE_FIELD_NAME_ARG "field_name"
> > @@ -49,6 +50,7 @@ static const char * const ice_valid_args[] = {
> > ICE_HW_DEBUG_MASK_ARG,
> > ICE_ONE_PPS_OUT_ARG,
> > ICE_RX_LOW_LATENCY_ARG,
> > + ICE_MAC_FILTER_DISABLE,
> > NULL
> > };
> >
> > @@ -962,8 +964,13 @@ ice_add_mac_filter(struct ice_vsi *vsi, struct
> > rte_ether_addr *mac_addr)
> > struct ice_mac_filter *f;
> > struct LIST_HEAD_TYPE list_head;
> > struct ice_hw *hw = ICE_VSI_TO_HW(vsi);
> > + struct ice_adapter *ad = (struct ice_adapter *)hw->back;
> > int ret = 0;
> >
> > + if (ad->devargs.mac_filter_disable == 1) {
> > + PMD_DRV_LOG(ERR, "This MAC filter is disabled.");
> > + return 0;
> > + }
> > /* If it's added and configured, return */
> > f = ice_find_mac_filter(vsi, mac_addr);
> > if (f) {
> > @@ -2075,6 +2082,11 @@ static int ice_parse_devargs(struct rte_eth_dev
> > *dev)
> > if (ret)
> > goto bail;
> >
> > + ret = rte_kvargs_process(kvlist, ICE_MAC_FILTER_DISABLE,
> > + &parse_bool, &ad-
> > >devargs.mac_filter_disable);
> > + if (ret)
> > + goto bail;
> > +
> > ret = rte_kvargs_process(kvlist, ICE_HW_DEBUG_MASK_ARG,
> > &parse_u64, &ad->hw.debug_mask);
> > if (ret)
> > @@ -6050,6 +6062,7 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ice,
> > ICE_PROTO_XTR_ARG
> > "=[queue:]"
> > ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
> > ICE_PIPELINE_MODE_SUPPORT_ARG "=<0|1>"
> > + ICE_MAC_FILTER_DISABLE "=<0|1>"
> > ICE_RX_LOW_LATENCY_ARG "=<0|1>");
> >
> > RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE); diff --git
> > a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h index
> > c8311be179..b4036d5fb0 100644
> > --- a/drivers/net/ice/ice_ethdev.h
> > +++ b/drivers/net/ice/ice_ethdev.h
> > @@ -563,6 +563,7 @@ struct ice_devargs {
> > int safe_mode_support;
> > uint8_t proto_xtr_dflt;
> > int pipe_mode_support;
[PATCH v3 1/2] eal: add nonnull and access function attributes
Add "nonnull" function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.
Add "access" function attribute to tell the compiler how a function
accesses its pointer arguments.
Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.
v3:
* No changes.
v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
GCC_VERSION being undefined.
* Try to fix Doxygen compliants.
Signed-off-by: Morten Brørup
Acked-by: Tyler Retzlaff
Reviewed-by: Ruifeng Wang
---
lib/eal/arm/include/rte_memcpy_32.h | 8
lib/eal/arm/include/rte_memcpy_64.h | 6 ++
lib/eal/include/rte_common.h| 29 +
lib/eal/ppc/include/rte_memcpy.h| 3 +++
lib/eal/x86/include/rte_memcpy.h| 6 ++
5 files changed, 52 insertions(+)
diff --git a/lib/eal/arm/include/rte_memcpy_32.h
b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..f454c06eca 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -14,6 +14,8 @@ extern "C" {
#include "generic/rte_memcpy.h"
+#include
+
#ifdef RTE_ARCH_ARM_NEON_MEMCPY
#ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy((dst), (src), (n)) : \
rte_memcpy_func((dst), (src), (n)); })
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
{
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy(dst, src, 256);
}
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy(void *dst, const void *src, size_t n)
{
diff --git a/lib/eal/arm/include/rte_memcpy_64.h
b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..55cbe07733 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src,
size_t n)
}
#if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline
void *rte_memcpy(void *dst, const void *src, size_t n)
{
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
}
#else
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline
void *rte_memcpy(void *dst, const void *src, size_t n)
{
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..6e4011aa85 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
__attribute__((format(printf, format_index, first_arg)))
#endif
+/**
+ * Check pointer arguments at compile-time.
+ *
+ * @param ...
+ *Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+ __attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler about the access mode of a pointer argument.
+ *
+ * @param access_mode
+ *Access mode: read_only, read_write, write_only, or none.
+ * @param ...
+ *Parameter index of pointer argument.
+ *Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_access_param(access_mode, ...) \
+ __attribute__((access(access_mode, __VA_ARGS__)))
+#else
+#define __rte_access_param(access_mode, ...)
+#endif
+
/**
* Tells compiler that the function returns a value that points to
* memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..59a5d86948 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy((dst), (src), (n)) : \
rte_memcpy_func((dst), (src), (n)); })
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
{
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..2e7161e4e7 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
* @return
* Pointer to the destination data.
*/
+__rte_nonnull_params(1, 2
[PATCH v3 2/2] net/bnx2x: fix warnings about rte_memcopy lengths
Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.
Added type casts where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings.
Bugzilla ID: 1146
Signed-off-by: Morten Brørup
v3:
* First patch in series.
---
drivers/net/bnx2x/bnx2x_stats.c | 6 +++---
drivers/net/bnx2x/bnx2x_vfpf.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..a5a7aa3b40 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -819,7 +819,7 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
rte_memcpy(old, new, sizeof(struct nig_stats));
-rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
+rte_memcpy((struct mac_stx *)&(estats->rx_stat_ifhcinbadoctets_hi),
&(pstats->mac_stx[1]),
sizeof(struct mac_stx));
estats->brb_drop_hi = pstats->brb_drop_hi;
estats->brb_drop_lo = pstats->brb_drop_lo;
@@ -1492,9 +1492,9 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
if (!CHIP_IS_E3(sc)) {
REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
- &(sc->port.old_nig_stats.egress_mac_pkt0_lo),
2);
+ (uint32_t
*)&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
- &(sc->port.old_nig_stats.egress_mac_pkt1_lo),
2);
+ (uint32_t
*)&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
}
/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..87631c76ca 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac,
sc->old_bulletin.mac, ETH_ALEN))
rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
if (valid_bitmap & (1 << VLAN_VALID))
- rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+ rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
sizeof(bull->vlan));
sc->old_bulletin = *bull;
--
2.17.1
[PATCH v4 1/2] eal: add nonnull and access function attributes
Add "nonnull" function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.
Add "access" function attribute to tell the compiler how a function
accesses its pointer arguments.
Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.
v4:
* No changes.
v3:
* No changes.
v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
GCC_VERSION being undefined.
* Try to fix Doxygen compliants.
Signed-off-by: Morten Brørup
Acked-by: Tyler Retzlaff
Reviewed-by: Ruifeng Wang
---
lib/eal/arm/include/rte_memcpy_32.h | 8
lib/eal/arm/include/rte_memcpy_64.h | 6 ++
lib/eal/include/rte_common.h| 29 +
lib/eal/ppc/include/rte_memcpy.h| 3 +++
lib/eal/x86/include/rte_memcpy.h| 6 ++
5 files changed, 52 insertions(+)
diff --git a/lib/eal/arm/include/rte_memcpy_32.h
b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..f454c06eca 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -14,6 +14,8 @@ extern "C" {
#include "generic/rte_memcpy.h"
+#include
+
#ifdef RTE_ARCH_ARM_NEON_MEMCPY
#ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy((dst), (src), (n)) : \
rte_memcpy_func((dst), (src), (n)); })
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
{
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy(dst, src, 256);
}
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy(void *dst, const void *src, size_t n)
{
diff --git a/lib/eal/arm/include/rte_memcpy_64.h
b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..55cbe07733 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src,
size_t n)
}
#if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline
void *rte_memcpy(void *dst, const void *src, size_t n)
{
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
}
#else
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline
void *rte_memcpy(void *dst, const void *src, size_t n)
{
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..6e4011aa85 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
__attribute__((format(printf, format_index, first_arg)))
#endif
+/**
+ * Check pointer arguments at compile-time.
+ *
+ * @param ...
+ *Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+ __attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler about the access mode of a pointer argument.
+ *
+ * @param access_mode
+ *Access mode: read_only, read_write, write_only, or none.
+ * @param ...
+ *Parameter index of pointer argument.
+ *Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_access_param(access_mode, ...) \
+ __attribute__((access(access_mode, __VA_ARGS__)))
+#else
+#define __rte_access_param(access_mode, ...)
+#endif
+
/**
* Tells compiler that the function returns a value that points to
* memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..59a5d86948 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy((dst), (src), (n)) : \
rte_memcpy_func((dst), (src), (n)); })
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
{
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..2e7161e4e7 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
* @return
* Pointer to the destination data.
*/
+__rte_n
[PATCH v4 2/2] net/bnx2x: fix warnings about rte_memcpy lengths
Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.
Added type casts where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings.
Bugzilla ID: 1146
Signed-off-by: Morten Brørup
v4:
* Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
v3:
* First patch in series.
---
drivers/net/bnx2x/bnx2x_stats.c | 11 +++
drivers/net/bnx2x/bnx2x_vfpf.c | 2 +-
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..1c44504663 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
rte_memcpy(old, new, sizeof(struct nig_stats));
-rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
- sizeof(struct mac_stx));
+rte_memcpy(RTE_PTR_ADD(estats,
+ offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
+ &(pstats->mac_stx[1]), sizeof(struct mac_stx));
estats->brb_drop_hi = pstats->brb_drop_hi;
estats->brb_drop_lo = pstats->brb_drop_lo;
@@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
if (!CHIP_IS_E3(sc)) {
REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
- &(sc->port.old_nig_stats.egress_mac_pkt0_lo),
2);
+ RTE_PTR_ADD(&(sc->port.old_nig_stats),
+ offsetof(struct nig_stats,
egress_mac_pkt0_lo)), 2);
REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
- &(sc->port.old_nig_stats.egress_mac_pkt1_lo),
2);
+ RTE_PTR_ADD(&(sc->port.old_nig_stats),
+ offsetof(struct nig_stats,
egress_mac_pkt1_lo)), 2);
}
/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..87631c76ca 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac,
sc->old_bulletin.mac, ETH_ALEN))
rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
if (valid_bitmap & (1 << VLAN_VALID))
- rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+ rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
sizeof(bull->vlan));
sc->old_bulletin = *bull;
--
2.17.1
dlb2 weird rte_memcpy
Timothy, What is the purpose of [1]? Is it superfluous, or should ".hw_rsrc_max" be removed from the source pointer, or is the size wrong? [1]: https://elixir.bootlin.com/dpdk/latest/source/drivers/event/dlb2/dlb2.c#L280 Building with access attributes to rte_memcpy() [2] causes this error: ../drivers/event/dlb2/dlb2.c: In function 'dlb2_hw_query_resources': ../drivers/event/dlb2/dlb2.c:280:9: error: 'rte_memcpy' reading 40 bytes from a region of size 32 [-Werror=stringop-overread] 280 | rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max, sizeof(*dlb2_info)); | ^~~~ In file included from ../drivers/event/dlb2/dlb2.c:34: ../drivers/event/dlb2/dlb2_priv.h:194:30: note: source object 'hw_rsrc_max' of size 32 194 | struct dlb2_hw_rsrcs hw_rsrc_max; | ^~~ In file included from ../lib/mempool/rte_mempool.h:48, from ../lib/eventdev/rte_eventdev.h:217, from ../drivers/event/dlb2/dlb2.c:22: ../lib/eal/x86/include/rte_memcpy.h:869:1: note: in a call to function 'rte_memcpy' declared with attribute 'access (read_only, 2, 3)' 869 | rte_memcpy(void *dst, const void *src, size_t n) | ^~ cc1: all warnings being treated as errors [2]: https://patchwork.dpdk.org/project/dpdk/patch/20221228114043.98739-1...@smartsharesystems.com/ Med venlig hilsen / Kind regards, -Morten Brørup
Re: dumpcap, interfaces, and promiscuous mode
I would like to respectfully ask that you please re-read my initial email.
Unfortunately the interface selection issue I describe is not resolved in
22.11 (currently running). It is also easily observed by reviewing the
current code (once one knows to look for it). I'll be the first to admit I
did not catch it when looking at the patch. To try and summarize it again
here, it is trying to store an array of interfaces (multiple -i options)
into a single char variable. The only interface that is persisted to
selection is the last one specified because it overwrites the
previously saved interface(s). If the intent is to only support capturing
on a single interface then the asterisk option should be removed, it should
become an error to specify the interface multiple times, and the associated
documentation should be updated to reflect this. However, I do not believe
the intent is to limit capture to a single interface and would not support
such a change.
I can understand modeling dumpcap off the wireshark version but DPDK brings
its own unique capabilities and I believe those need to be accounted for
too.
Looking at the man page of wireshark's dumpcap, it indicates that the "-p"
option can be specified multiple times and it needs to be relative to the
interface [1], if it is not it may be ignored. Implementing this change
seems like it would bring DPDK dumpcap closer to the behavior of
wireshark's dumpcap when specifying the interface's promiscuous mode.
The above promiscuous mode change does not address the potential issue of
stopping a capture and it changing an interface's promiscuous mode for the
main application that continues to run. The only path forward here that I
can see is storing the initial promiscuous mode state on each interface
that a capture is occurring on and checking that to determine what should
be done when stopping dumpcap.
This leaves the last question of being able to just inherit the main
process's promiscuous state so that a user doesn't necessarily need to know
which interfaces are in which state when starting dumpcap to troubleshoot
something. Tying in to the previous about storing the state, it would
potentially avoid a user starting all interfaces in promiscuous mode which
might result in different traffic in the capture than the application
normally sees.
Finally after looking at the wireshark dumpcap man page, I believe there is
a new issue here as well. The man page states that if a capture will occur
on multiple interfaces, the file will be saved in pcapng format. Quickly
at the code it does not look like this is the current behavior of DPDK
dumpcap.
As mentioned before I am happy to try and work on some of these changes,
but would like to have something of a plan before starting that work.
Cheers,
Ben Magistro
[1] https://www.wireshark.org/docs/man-pages/dumpcap.html
On Tue, Dec 27, 2022 at 11:43 AM Stephen Hemminger <
step...@networkplumber.org> wrote:
> On Tue, 27 Dec 2022 09:26:14 -0500
> Ben Magistro wrote:
>
> > I'd like to start out by saying what I believed to be a simple change
> > (a8dde09 ("app/dumpcap: allow help/version without primary process"))
> seems
> > to have had more ripples than I anticipated. I'd like to just get a
> > conversation going here before developing/submitting a patch. I think
> this
> > will likely need to be at least two patches just to keep scope in check.
> >
> > With regard to interface selection, the most recent patch (7f3623a
> > ("app/dumpcap: fix select interface")) breaks capturing on
> > multiple interfaces. You can still specify the `-i` option as many times
> > as you would like but only the last entry is used in the capture
> selection
> > as each entry just overwrites the previous entry. I believe this needs
> to
> > be flipped to an array or similar structure that can have entries
> > appended to it as the arguments are processed. Selecting all interfaces
> > with the asterisk seems to be unaffected but could also result in
> capturing
> > traffic on unnecessary interfaces.
> >
> > With regard to promiscuous mode, there are two areas of concern here.
> The
> > first is this flag is global and not per interface. I can envision a
> > scenario where you might want to capture on one interface in promiscuous
> > mode and on a second not in promiscuous mode. My first thought is to
> make
> > this option follow the interface parameter then when parsing arguments so
> > that it can be associated with a specific interface. The second is that
> if
> > I run a capture in promiscuous mode and then stop the capture, it will
> also
> > disable promiscuous mode. Generally I think I would agree with this
> > behavior as it follows how a typical call to tcpdump should behave. The
> > concern here though is that that the main process may have been
> > started/running with promiscuous mode and stopping a capture would change
> > that mode for the main process. My first thought here is to query the
> > initial st
RE: Mysterious CI/IOL failures in Patchwork
Hi Owen, Thanks for fixing the dashboard. It looks like the email test result sender needs to have some environments added too, please refer to the email below – testing fail, but all say PASS. Test-Label: iol-testing Test-Status: FAILURE http://dpdk.org/patch/121449 _Testing issues_ Submitter: Morten Brørup Date: Wednesday, December 28 2022 11:40:43 DPDK git baseline: Repo:dpdk Branch: master CommitID:373f4c7de8ff350548cacc3d56e788461677f2c7 121448-121449 --> testing fail Test environment and result as below: +++--++ | Environment | dpdk_meson_compile | dpdk_mingw64_compile | dpdk_unit_test | +++==++ | Windows Server 2019| PASS | PASS | PASS | +++--++ | Ubuntu 20.04 ARM GCC Native| PASS | SKIPPED | PASS | +++--++ | FreeBSD 13 | PASS | SKIPPED | SKIPPED| +++--++ | Ubuntu 20.04 ARM Clang Native | PASS | SKIPPED | PASS | +++--++ | Ubuntu 22.04 | PASS | SKIPPED | SKIPPED| +++--++ | CentOS Stream 8| PASS | SKIPPED | PASS | +++--++ | Debian Buster | PASS | SKIPPED | SKIPPED| +++--++ | RHEL 7 | PASS | SKIPPED | SKIPPED| +++--++ | Debian Bullseye| PASS | SKIPPED | SKIPPED| +++--++ | CentOS Stream 9| PASS | SKIPPED | SKIPPED| +++--++ | Ubuntu 20.04 aarch32 GCC Cross Compile | PASS | SKIPPED | SKIPPED| +++--++ | openSUSE Leap 15 | PASS | SKIPPED | SKIPPED| +++--++ | RHEL8 | PASS | SKIPPED | SKIPPED| +++--++ | Ubuntu 20.04 | PASS | SKIPPED | SKIPPED| +++--++ Med venlig hilsen / Kind regards, -Morten Brørup From: Owen Hilyard [mailto:ohily...@iol.unh.edu] Sent: Wednesday, 7 December 2022 16.21 To: Morten Brørup Cc: c...@dpdk.org; dev@dpdk.org Subject: Re: Mysterious CI/IOL failures in Patchwork Hello Morten, It looks like our Fedora 35 and 36 environments were erroneously hidden from public view. This has been fixed and you should now be able to see the issues on the dashboard at the link you provided. Also, please CC dpdk...@iol.unh.edu when you want to contact the community lab. That address will send the email to everyone involved with DPDK here at UNH so you will usually get faster response times. Sorry for any inconvenience Owen Hilyard
[PATCH v5 1/4] eal: add nonnull and access function attributes
Add "nonnull" function attribute to help the compiler detect a NULL
pointer being passed to a function not accepting NULL pointers as an
argument at build time.
Add "access" function attribute to tell the compiler how a function
accesses its pointer arguments.
Add these attributes to the rte_memcpy() function, as the first in
hopefully many to come.
v5:
* No changes.
v4:
* No changes.
v3:
* No changes.
v2:
* Only define "nonnull" for GCC and CLANG.
* Append _param/_params to prepare for possible future attributes
attached directly to the individual parameters, like __rte_unused.
* Use RTE_TOOLCHAIN_GCC instead of RTE_CC_GCC, to fix complaints about
GCC_VERSION being undefined.
* Try to fix Doxygen compliants.
Signed-off-by: Morten Brørup
Acked-by: Tyler Retzlaff
Reviewed-by: Ruifeng Wang
---
lib/eal/arm/include/rte_memcpy_32.h | 8
lib/eal/arm/include/rte_memcpy_64.h | 6 ++
lib/eal/include/rte_common.h| 29 +
lib/eal/ppc/include/rte_memcpy.h| 3 +++
lib/eal/x86/include/rte_memcpy.h| 6 ++
5 files changed, 52 insertions(+)
diff --git a/lib/eal/arm/include/rte_memcpy_32.h
b/lib/eal/arm/include/rte_memcpy_32.h
index fb3245b59c..f454c06eca 100644
--- a/lib/eal/arm/include/rte_memcpy_32.h
+++ b/lib/eal/arm/include/rte_memcpy_32.h
@@ -14,6 +14,8 @@ extern "C" {
#include "generic/rte_memcpy.h"
+#include
+
#ifdef RTE_ARCH_ARM_NEON_MEMCPY
#ifndef __ARM_NEON
@@ -125,6 +127,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy((dst), (src), (n)) : \
rte_memcpy_func((dst), (src), (n)); })
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
{
@@ -290,6 +295,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy(dst, src, 256);
}
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy(void *dst, const void *src, size_t n)
{
diff --git a/lib/eal/arm/include/rte_memcpy_64.h
b/lib/eal/arm/include/rte_memcpy_64.h
index 85ad587bd3..55cbe07733 100644
--- a/lib/eal/arm/include/rte_memcpy_64.h
+++ b/lib/eal/arm/include/rte_memcpy_64.h
@@ -282,6 +282,9 @@ void rte_memcpy_ge64(uint8_t *dst, const uint8_t *src,
size_t n)
}
#if RTE_CACHE_LINE_SIZE >= 128
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline
void *rte_memcpy(void *dst, const void *src, size_t n)
{
@@ -303,6 +306,9 @@ void *rte_memcpy(void *dst, const void *src, size_t n)
}
#else
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static __rte_always_inline
void *rte_memcpy(void *dst, const void *src, size_t n)
{
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 15765b408d..6e4011aa85 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -149,6 +149,35 @@ typedef uint16_t unaligned_uint16_t;
__attribute__((format(printf, format_index, first_arg)))
#endif
+/**
+ * Check pointer arguments at compile-time.
+ *
+ * @param ...
+ *Comma separated list of parameter indexes of pointer arguments.
+ */
+#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
+#define __rte_nonnull_params(...) \
+ __attribute__((nonnull(__VA_ARGS__)))
+#else
+#define __rte_nonnull_params(...)
+#endif
+
+/**
+ * Tells compiler about the access mode of a pointer argument.
+ *
+ * @param access_mode
+ *Access mode: read_only, read_write, write_only, or none.
+ * @param ...
+ *Parameter index of pointer argument.
+ *Optionally followeded by comma and parameter index of size argument.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100400)
+#define __rte_access_param(access_mode, ...) \
+ __attribute__((access(access_mode, __VA_ARGS__)))
+#else
+#define __rte_access_param(access_mode, ...)
+#endif
+
/**
* Tells compiler that the function returns a value that points to
* memory, where the size is given by the one or two arguments.
diff --git a/lib/eal/ppc/include/rte_memcpy.h b/lib/eal/ppc/include/rte_memcpy.h
index 6f388c0234..59a5d86948 100644
--- a/lib/eal/ppc/include/rte_memcpy.h
+++ b/lib/eal/ppc/include/rte_memcpy.h
@@ -84,6 +84,9 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
memcpy((dst), (src), (n)) : \
rte_memcpy_func((dst), (src), (n)); })
+__rte_nonnull_params(1, 2)
+__rte_access_param(write_only, 1, 3)
+__rte_access_param(read_only, 2, 3)
static inline void *
rte_memcpy_func(void *dst, const void *src, size_t n)
{
diff --git a/lib/eal/x86/include/rte_memcpy.h b/lib/eal/x86/include/rte_memcpy.h
index d4d7a5cfc8..2e7161e4e7 100644
--- a/lib/eal/x86/include/rte_memcpy.h
+++ b/lib/eal/x86/include/rte_memcpy.h
@@ -42,6 +42,9 @@ extern "C" {
* @return
* Pointer to the destination d
[PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths
Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.
Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings.
Bugzilla ID: 1146
Signed-off-by: Morten Brørup
v5:
* No changes.
v4:
* Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
v3:
* First patch in series.
---
drivers/net/bnx2x/bnx2x_stats.c | 11 +++
drivers/net/bnx2x/bnx2x_vfpf.c | 2 +-
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..1c44504663 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
rte_memcpy(old, new, sizeof(struct nig_stats));
-rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
- sizeof(struct mac_stx));
+rte_memcpy(RTE_PTR_ADD(estats,
+ offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
+ &(pstats->mac_stx[1]), sizeof(struct mac_stx));
estats->brb_drop_hi = pstats->brb_drop_hi;
estats->brb_drop_lo = pstats->brb_drop_lo;
@@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
if (!CHIP_IS_E3(sc)) {
REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
- &(sc->port.old_nig_stats.egress_mac_pkt0_lo),
2);
+ RTE_PTR_ADD(&(sc->port.old_nig_stats),
+ offsetof(struct nig_stats,
egress_mac_pkt0_lo)), 2);
REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
- &(sc->port.old_nig_stats.egress_mac_pkt1_lo),
2);
+ RTE_PTR_ADD(&(sc->port.old_nig_stats),
+ offsetof(struct nig_stats,
egress_mac_pkt1_lo)), 2);
}
/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..87631c76ca 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac,
sc->old_bulletin.mac, ETH_ALEN))
rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
if (valid_bitmap & (1 << VLAN_VALID))
- rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+ rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
sizeof(bull->vlan));
sc->old_bulletin = *bull;
--
2.17.1
[PATCH v5 3/4] event/dlb2: remove superfluous rte_memcpy
Copying with the same src and dst address has no effect; removed to
avoid compiler warning.
Signed-off-by: Morten Brørup
v5:
* First patch in series.
---
drivers/event/dlb2/dlb2.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
index 60c5cd4804..03d32c779f 100644
--- a/drivers/event/dlb2/dlb2.c
+++ b/drivers/event/dlb2/dlb2.c
@@ -215,7 +215,6 @@ static int
dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
{
struct dlb2_hw_dev *handle = &dlb2->qm_instance;
- struct dlb2_hw_resource_info *dlb2_info = &handle->info;
int num_ldb_ports;
int ret;
@@ -277,8 +276,6 @@ dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
handle->info.hw_rsrc_max.reorder_window_size =
dlb2->hw_rsrc_query_results.num_hist_list_entries;
- rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max, sizeof(*dlb2_info));
-
return 0;
}
--
2.17.1
[PATCH v5 4/4] net/mlx5: fix warning about rte_memcpy length
Use RTE_PTR_ADD where copying to the offset of a field in a structure holding multiple fields, to avoid compiler warnings. Signed-off-by: Morten Brørup v5: * First patch in series. --- drivers/net/mlx5/mlx5_flow_dv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c index 62c38b87a1..dd9f5fda1a 100644 --- a/drivers/net/mlx5/mlx5_flow_dv.c +++ b/drivers/net/mlx5/mlx5_flow_dv.c @@ -5662,7 +5662,7 @@ flow_dv_modify_create_cb(void *tool_ctx, void *cb_ctx) "cannot allocate resource memory"); return NULL; } - rte_memcpy(&entry->ft_type, + rte_memcpy(RTE_PTR_ADD(entry, offsetof(typeof(*entry), ft_type)), RTE_PTR_ADD(ref, offsetof(typeof(*ref), ft_type)), key_len + data_len); if (entry->ft_type == MLX5DV_FLOW_TABLE_TYPE_FDB) -- 2.17.1
Re: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths
On Wed, Dec 28, 2022, 16:10 Morten Brørup wrote:
> Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
> VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> after the field, so copying 2 byte too many is effectively harmless.
>
It is a small nitpick but why use rte_memcpy for a 2 byte / half-word copy?
Shouldn't assignment with casts be enough?
> There is no need to backport this patch.
>
> Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
> structure holding multiple fields, to avoid compiler warnings.
>
> Bugzilla ID: 1146
>
> Signed-off-by: Morten Brørup
>
> v5:
> * No changes.
> v4:
> * Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
> v3:
> * First patch in series.
> ---
> drivers/net/bnx2x/bnx2x_stats.c | 11 +++
> drivers/net/bnx2x/bnx2x_vfpf.c | 2 +-
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/bnx2x/bnx2x_stats.c
> b/drivers/net/bnx2x/bnx2x_stats.c
> index c07b01510a..1c44504663 100644
> --- a/drivers/net/bnx2x/bnx2x_stats.c
> +++ b/drivers/net/bnx2x/bnx2x_stats.c
> @@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
>
> rte_memcpy(old, new, sizeof(struct nig_stats));
>
> -rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi),
> &(pstats->mac_stx[1]),
> - sizeof(struct mac_stx));
> +rte_memcpy(RTE_PTR_ADD(estats,
> + offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
> + &(pstats->mac_stx[1]), sizeof(struct mac_stx));
> estats->brb_drop_hi = pstats->brb_drop_hi;
> estats->brb_drop_lo = pstats->brb_drop_lo;
>
> @@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
> REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
> if (!CHIP_IS_E3(sc)) {
> REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
> -
> &(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
> + RTE_PTR_ADD(&(sc->port.old_nig_stats),
> + offsetof(struct nig_stats,
> egress_mac_pkt0_lo)), 2);
> REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
> -
> &(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
> + RTE_PTR_ADD(&(sc->port.old_nig_stats),
> + offsetof(struct nig_stats,
> egress_mac_pkt1_lo)), 2);
> }
>
> /* function stats */
> diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c
> b/drivers/net/bnx2x/bnx2x_vfpf.c
> index 63953c2979..87631c76ca 100644
> --- a/drivers/net/bnx2x/bnx2x_vfpf.c
> +++ b/drivers/net/bnx2x/bnx2x_vfpf.c
> @@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
> if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac,
> sc->old_bulletin.mac, ETH_ALEN))
> rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
> if (valid_bitmap & (1 << VLAN_VALID))
> - rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> RTE_VLAN_HLEN);
> + rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> sizeof(bull->vlan));
>
> sc->old_bulletin = *bull;
>
> --
> 2.17.1
>
>
RE: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths
From: Stanisław Kardach [mailto:k...@semihalf.com]
Sent: Wednesday, 28 December 2022 17.14
> On Wed, Dec 28, 2022, 16:10 Morten Brørup wrote:
> > Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
> > VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> > after the field, so copying 2 byte too many is effectively harmless.
> It is a small nitpick but why use rte_memcpy for a 2 byte / half-word copy?
> Shouldn't assignment with casts be enough?
Absolutely. It would also have prevented the bug to begin with.
But in order to keep the changes minimal, I kept the rte_memcpy().
> > There is no need to backport this patch.
> >
> > Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
> > structure holding multiple fields, to avoid compiler warnings.
> >
> > Bugzilla ID: 1146
> >
> > Signed-off-by: Morten Brørup
> >
> > v5:
> > * No changes.
> > v4:
> > * Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
> > v3:
> > * First patch in series.
> > ---
> > drivers/net/bnx2x/bnx2x_stats.c | 11 +++
> > drivers/net/bnx2x/bnx2x_vfpf.c | 2 +-
> > 2 files changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/bnx2x/bnx2x_stats.c
> > b/drivers/net/bnx2x/bnx2x_stats.c
> > index c07b01510a..1c44504663 100644
> > --- a/drivers/net/bnx2x/bnx2x_stats.c
> > +++ b/drivers/net/bnx2x/bnx2x_stats.c
> > @@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
> >
> > rte_memcpy(old, new, sizeof(struct nig_stats));
> >
> > -rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi),
> > &(pstats->mac_stx[1]),
> > - sizeof(struct mac_stx));
> > +rte_memcpy(RTE_PTR_ADD(estats,
> > + offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
> > + &(pstats->mac_stx[1]), sizeof(struct mac_stx));
> > estats->brb_drop_hi = pstats->brb_drop_hi;
> > estats->brb_drop_lo = pstats->brb_drop_lo;
> >
> > @@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
> > REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
> > if (!CHIP_IS_E3(sc)) {
> > REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
> > -
> > &(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
> > + RTE_PTR_ADD(&(sc->port.old_nig_stats),
> > + offsetof(struct nig_stats,
> > egress_mac_pkt0_lo)), 2);
> > REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
> > -
> > &(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
> > + RTE_PTR_ADD(&(sc->port.old_nig_stats),
> > + offsetof(struct nig_stats,
> > egress_mac_pkt1_lo)), 2);
> > }
> >
> > /* function stats */
> > diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
> > index 63953c2979..87631c76ca 100644
> > --- a/drivers/net/bnx2x/bnx2x_vfpf.c
> > +++ b/drivers/net/bnx2x/bnx2x_vfpf.c
> > @@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
> > if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac,
> > sc->old_bulletin.mac, ETH_ALEN))
> > rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
> > if (valid_bitmap & (1 << VLAN_VALID))
> > - rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> > RTE_VLAN_HLEN);
> > + rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> > sizeof(bull->vlan));
As Stanisław mentioned, this might as well be:
if (valid_bitmap & (1 << VLAN_VALID))
- rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+ bull->vlan = sc->old_bulletin.vlan;
> >
> > sc->old_bulletin = *bull;
> >
> > --
> > 2.17.1
[RFC] ethdev: sharing indirect actions between ports
The RTE Flow API implements the concept of shared objects,
known as indirect actions (RTE_FLOW_ACTION_TYPE_INDIRECT).
An application can create the indirect action of desired
type and configuration with rte_flow_action_handle_create
call and then specify the obtained action handle in multiple
flows.
The initial concept supposes the action handle has strict
attachment to the port it was created on and to be used
exclusively in the flows being installed on the port.
Nowadays the multipath network topologies are quite common,
packets belonging to the same connection might arrive and
be sent over multiple ports, and there is the raising demand
to handle these "spread" connections. To fulfil this demand
it is proposed to extend indirect action sharing across the
multiple ports. This kind of sharing would be extremely useful
for the meters and counters, allowing to manage the single
connection over the multiple ports.
This cross-port object sharing is hard to implement in
generic way merely with software on the upper layers, but
can be provided by the driver over the single hardware
instance, where multiple ports reside on the same physical
NIC and share the same hardware context.
To allow this action sharing application should specify
the "host port" during flow configuring to claim the intention
to share the indirect actions. All indirect actions reside within
"host port" context and can be shared in flows being installed
on the host port and on all the ports referencing this one.
If sharing between host and port being configured is not supported
the configuration should be rejected with error. There might be
multiple independent (mutual exclusive) sharing domains with
dedicated host and referencing ports.
To manage the shared indirect action any port from sharing domain
can be specified. To share or not the created action is up to
application, no API change is needed.
Signed-off-by: Viacheslav Ovsiienko
---
lib/ethdev/rte_flow.c | 6 ++
lib/ethdev/rte_flow.h | 11 +++
2 files changed, 17 insertions(+)
diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
index 7d0c24366c..692d37925a 100644
--- a/lib/ethdev/rte_flow.c
+++ b/lib/ethdev/rte_flow.c
@@ -1476,6 +1476,12 @@ rte_flow_configure(uint16_t port_id,
RTE_FLOW_LOG(ERR, "Port %"PRIu16" queue info is NULL.\n",
port_id);
return -EINVAL;
}
+ if ((port_attr->flags & RTE_FLOW_PORT_FLAG_SHARE_INDIRECT) &&
+!rte_eth_dev_is_valid_port(port_attr->host_port_id)) {
+ return rte_flow_error_set(error, ENODEV,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+ NULL, rte_strerror(ENODEV));
+ }
if (likely(!!ops->configure)) {
ret = ops->configure(dev, port_attr, nb_queue, queue_attr,
error);
if (ret == 0)
diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index b60987db4b..c784f4ec30 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -4903,6 +4903,13 @@ rte_flow_info_get(uint16_t port_id,
struct rte_flow_queue_info *queue_info,
struct rte_flow_error *error);
+/**
+ * Indicate all steering objects should be created on contexts
+ * of the host port, providing indirect object sharing beetween
+ * ports.
+ */
+#define RTE_FLOW_PORT_FLAG_SHARE_INDIRECT RTE_BIT32(0)
+
/**
* @warning
* @b EXPERIMENTAL: this API may change without prior notice.
@@ -4932,6 +4939,10 @@ struct rte_flow_port_attr {
* @see RTE_FLOW_ACTION_TYPE_CONNTRACK
*/
uint32_t nb_conn_tracks;
+ /**
+* Port to base shared objects on.
+*/
+ uint16_t host_port_id;
/**
* Port flags (RTE_FLOW_PORT_FLAG_*).
*/
--
2.18.1
Re: [PATCH v5 3/4] event/dlb2: remove superfluous rte_memcpy
On Wed, 28 Dec 2022 16:10:18 +0100
Morten Brørup wrote:
> Copying with the same src and dst address has no effect; removed to
> avoid compiler warning.
>
> Signed-off-by: Morten Brørup
>
> v5:
> * First patch in series.
> ---
> drivers/event/dlb2/dlb2.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 60c5cd4804..03d32c779f 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -215,7 +215,6 @@ static int
> dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
> {
> struct dlb2_hw_dev *handle = &dlb2->qm_instance;
> - struct dlb2_hw_resource_info *dlb2_info = &handle->info;
> int num_ldb_ports;
> int ret;
>
> @@ -277,8 +276,6 @@ dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
> handle->info.hw_rsrc_max.reorder_window_size =
> dlb2->hw_rsrc_query_results.num_hist_list_entries;
>
> - rte_memcpy(dlb2_info, &handle->info.hw_rsrc_max, sizeof(*dlb2_info));
> -
> return 0;
> }
Please add Fixes and CC author.
Fixes: e7c9971a857a ("event/dlb2: add probe-time hardware init")
Cc: timothy.mcdan...@intel.com
Acked-by: Stephen Hemminger
Re: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths
On Wed, 28 Dec 2022 17:38:56 +0100 Morten Brørup wrote: > From: Stanisław Kardach [mailto:k...@semihalf.com] > Sent: Wednesday, 28 December 2022 17.14 > > On Wed, Dec 28, 2022, 16:10 Morten Brørup > > wrote: > > > Bugfix: The vlan in the bulletin does not contain a VLAN header, only the > > > VLAN ID, so only copy 2 byte, not 4. The target structure has padding > > > after the field, so copying 2 byte too many is effectively harmless. > > It is a small nitpick but why use rte_memcpy for a 2 byte / half-word copy? > > Shouldn't assignment with casts be enough? > > Absolutely. It would also have prevented the bug to begin with. > But in order to keep the changes minimal, I kept the rte_memcpy(). For small fixed values compiler can optimize memcpy into one instruction. Not so with current rte_memcpy
Re: dumpcap, interfaces, and promiscuous mode
On Wed, 28 Dec 2022 09:12:12 -0500
Ben Magistro wrote:
> I would like to respectfully ask that you please re-read my initial email.
>
> Unfortunately the interface selection issue I describe is not resolved in
> 22.11 (currently running). It is also easily observed by reviewing the
> current code (once one knows to look for it). I'll be the first to admit I
> did not catch it when looking at the patch. To try and summarize it again
> here, it is trying to store an array of interfaces (multiple -i options)
> into a single char variable. The only interface that is persisted to
> selection is the last one specified because it overwrites the
> previously saved interface(s). If the intent is to only support capturing
> on a single interface then the asterisk option should be removed, it should
> become an error to specify the interface multiple times, and the associated
> documentation should be updated to reflect this. However, I do not believe
> the intent is to limit capture to a single interface and would not support
> such a change.
>
> I can understand modeling dumpcap off the wireshark version but DPDK brings
> its own unique capabilities and I believe those need to be accounted for
> too.
>
> Looking at the man page of wireshark's dumpcap, it indicates that the "-p"
> option can be specified multiple times and it needs to be relative to the
> interface [1], if it is not it may be ignored. Implementing this change
> seems like it would bring DPDK dumpcap closer to the behavior of
> wireshark's dumpcap when specifying the interface's promiscuous mode.
>
> The above promiscuous mode change does not address the potential issue of
> stopping a capture and it changing an interface's promiscuous mode for the
> main application that continues to run. The only path forward here that I
> can see is storing the initial promiscuous mode state on each interface
> that a capture is occurring on and checking that to determine what should
> be done when stopping dumpcap.
>
> This leaves the last question of being able to just inherit the main
> process's promiscuous state so that a user doesn't necessarily need to know
> which interfaces are in which state when starting dumpcap to troubleshoot
> something. Tying in to the previous about storing the state, it would
> potentially avoid a user starting all interfaces in promiscuous mode which
> might result in different traffic in the capture than the application
> normally sees.
>
> Finally after looking at the wireshark dumpcap man page, I believe there is
> a new issue here as well. The man page states that if a capture will occur
> on multiple interfaces, the file will be saved in pcapng format. Quickly
> at the code it does not look like this is the current behavior of DPDK
> dumpcap.
>
> As mentioned before I am happy to try and work on some of these changes,
> but would like to have something of a plan before starting that work.
>
> Cheers,
>
> Ben Magistro
>
> [1] https://www.wireshark.org/docs/man-pages/dumpcap.html
>
> On Tue, Dec 27, 2022 at 11:43 AM Stephen Hemminger <
> step...@networkplumber.org> wrote:
>
> > On Tue, 27 Dec 2022 09:26:14 -0500
> > Ben Magistro wrote:
> >
> > > I'd like to start out by saying what I believed to be a simple change
> > > (a8dde09 ("app/dumpcap: allow help/version without primary process"))
> > seems
> > > to have had more ripples than I anticipated. I'd like to just get a
> > > conversation going here before developing/submitting a patch. I think
> > this
> > > will likely need to be at least two patches just to keep scope in check.
> > >
> > > With regard to interface selection, the most recent patch (7f3623a
> > > ("app/dumpcap: fix select interface")) breaks capturing on
> > > multiple interfaces. You can still specify the `-i` option as many times
> > > as you would like but only the last entry is used in the capture
> > selection
> > > as each entry just overwrites the previous entry. I believe this needs
> > to
> > > be flipped to an array or similar structure that can have entries
> > > appended to it as the arguments are processed. Selecting all interfaces
> > > with the asterisk seems to be unaffected but could also result in
> > capturing
> > > traffic on unnecessary interfaces.
> > >
> > > With regard to promiscuous mode, there are two areas of concern here.
> > The
> > > first is this flag is global and not per interface. I can envision a
> > > scenario where you might want to capture on one interface in promiscuous
> > > mode and on a second not in promiscuous mode. My first thought is to
> > make
> > > this option follow the interface parameter then when parsing arguments so
> > > that it can be associated with a specific interface. The second is that
> > if
> > > I run a capture in promiscuous mode and then stop the capture, it will
> > also
> > > disable promiscuous mode. Generally I think I would agree with this
> > > behavior as it foll
RE: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths
> From: Stephen Hemminger [mailto:step...@networkplumber.org] > Sent: Wednesday, 28 December 2022 18.03 > > On Wed, 28 Dec 2022 17:38:56 +0100 > Morten Brørup wrote: > > > From: Stanisław Kardach [mailto:k...@semihalf.com] > > Sent: Wednesday, 28 December 2022 17.14 > > > On Wed, Dec 28, 2022, 16:10 Morten Brørup > wrote: > > > > Bugfix: The vlan in the bulletin does not contain a VLAN header, > only the > > > > VLAN ID, so only copy 2 byte, not 4. The target structure has > padding > > > > after the field, so copying 2 byte too many is effectively > harmless. > > > It is a small nitpick but why use rte_memcpy for a 2 byte / half- > word copy? Shouldn't assignment with casts be enough? > > > > Absolutely. It would also have prevented the bug to begin with. > > But in order to keep the changes minimal, I kept the rte_memcpy(). > > For small fixed values compiler can optimize memcpy into one > instruction. > Not so with current rte_memcpy Good point, Stephen. I took another look at it, and the two byte rte_memcpy() is only used in a slow path function, so no need to optimize further. If the maintainers disagree, and want to optimize anyway, here's a proposal: /* check the mac address and VLAN and allocate memory if valid */ if (valid_bitmap & (1 << MAC_ADDR_VALID) && !rte_is_same_ether_addr( (const struct rte_ether_addr *)&bull->mac, (const struct rte_ether_addr *)&sc->old_bulletin.mac)) rte_ether_addr_copy((const struct rte_ether_addr *)&bull->mac, (struct rte_ether_addr *)&sc->link_params.mac_addr); if (valid_bitmap & (1 << VLAN_VALID)) bull->vlan = sc->old_bulletin.vlan;
[PATCH v2] net/mlx5: add stub functions to null drv ops
There are several functions implementation that queries the drv type
to understand which fops function to use.
In case the type is DV, the function gets the concrete DV function and
calls it.
In case it’s not, the function returns an error.
The current implementation is not flexible enough and will not support
future concrete functions in a good way.
This patch adds more stub functions that include error handling and
are assigned to the null drv ops struct.
This allows the functions to always call the virtual function without
basing the decision on a specific drv type.
Signed-off-by: Asaf Penso
---
drivers/net/mlx5/mlx5_flow.c | 88 +-
drivers/net/mlx5/mlx5_flow.h | 13 +
drivers/net/mlx5/mlx5_flow_verbs.c | 4 ++
3 files changed, 68 insertions(+), 37 deletions(-)
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index a0cf677fb0..a0779d810d 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -3786,6 +3786,46 @@ flow_null_sync_domain(struct rte_eth_dev *dev
__rte_unused,
return 0;
}
+int
+flow_null_get_aged_flows(struct rte_eth_dev *dev,
+ void **context __rte_unused,
+ uint32_t nb_contexts __rte_unused,
+ struct rte_flow_error *error __rte_unused)
+{
+ DRV_LOG(ERR, "port %u get aged flows is not supported.",
+ dev->data->port_id);
+ return -ENOTSUP;
+}
+
+uint32_t
+flow_null_counter_allocate(struct rte_eth_dev *dev)
+{
+ DRV_LOG(ERR, "port %u counter allocate is not supported.",
+ dev->data->port_id);
+ return 0;
+}
+
+void
+flow_null_counter_free(struct rte_eth_dev *dev,
+ uint32_t counter __rte_unused)
+{
+ DRV_LOG(ERR, "port %u counter free is not supported.",
+dev->data->port_id);
+}
+
+int
+flow_null_counter_query(struct rte_eth_dev *dev,
+ uint32_t counter __rte_unused,
+ bool clear __rte_unused,
+ uint64_t *pkts __rte_unused,
+ uint64_t *bytes __rte_unused,
+ void **action __rte_unused)
+{
+ DRV_LOG(ERR, "port %u counter query is not supported.",
+dev->data->port_id);
+ return -ENOTSUP;
+}
+
/* Void driver to protect from null pointer reference. */
const struct mlx5_flow_driver_ops mlx5_flow_null_drv_ops = {
.validate = flow_null_validate,
@@ -3796,6 +3836,10 @@ const struct mlx5_flow_driver_ops mlx5_flow_null_drv_ops
= {
.destroy = flow_null_destroy,
.query = flow_null_query,
.sync_domain = flow_null_sync_domain,
+ .get_aged_flows = flow_null_get_aged_flows,
+ .counter_alloc = flow_null_counter_allocate,
+ .counter_free = flow_null_counter_free,
+ .counter_query = flow_null_counter_query
};
/**
@@ -8352,17 +8396,10 @@ mlx5_flow_mtr_free(struct rte_eth_dev *dev, uint32_t
mtr_idx)
uint32_t
mlx5_counter_alloc(struct rte_eth_dev *dev)
{
- const struct mlx5_flow_driver_ops *fops;
struct rte_flow_attr attr = { .transfer = 0 };
- if (flow_get_drv_type(dev, &attr) == MLX5_FLOW_TYPE_DV) {
- fops = flow_get_drv_ops(MLX5_FLOW_TYPE_DV);
- return fops->counter_alloc(dev);
- }
- DRV_LOG(ERR,
- "port %u counter allocate is not supported.",
-dev->data->port_id);
- return 0;
+ return flow_get_drv_ops(flow_get_drv_type(dev, &attr))->counter_alloc
+ (dev);
}
/**
@@ -8376,17 +8413,9 @@ mlx5_counter_alloc(struct rte_eth_dev *dev)
void
mlx5_counter_free(struct rte_eth_dev *dev, uint32_t cnt)
{
- const struct mlx5_flow_driver_ops *fops;
struct rte_flow_attr attr = { .transfer = 0 };
- if (flow_get_drv_type(dev, &attr) == MLX5_FLOW_TYPE_DV) {
- fops = flow_get_drv_ops(MLX5_FLOW_TYPE_DV);
- fops->counter_free(dev, cnt);
- return;
- }
- DRV_LOG(ERR,
- "port %u counter free is not supported.",
-dev->data->port_id);
+ flow_get_drv_ops(flow_get_drv_type(dev, &attr))->counter_free(dev, cnt);
}
/**
@@ -8410,18 +8439,10 @@ int
mlx5_counter_query(struct rte_eth_dev *dev, uint32_t cnt,
bool clear, uint64_t *pkts, uint64_t *bytes, void **action)
{
- const struct mlx5_flow_driver_ops *fops;
struct rte_flow_attr attr = { .transfer = 0 };
- if (flow_get_drv_type(dev, &attr) == MLX5_FLOW_TYPE_DV) {
- fops = flow_get_drv_ops(MLX5_FLOW_TYPE_DV);
- return fops->counter_query(dev, cnt, clear, pkts,
- bytes, action);
- }
- DRV_LOG(ERR,
- "port %u counter query is not supported.",
-dev->data->port_id);
- return -ENOTSUP;
+ return flow_get_drv_ops(flow_get_drv_type(dev, &attr))->counter
[PATCH v3] net/iavf: add lock for vf commands
iavf admin queue commands aren't thread-safe. Bugs surrounding this
issue can manifest in a variety of ways but frequently pend_cmd is
over written. Simultaneously executing commands can result in a
misconfigured device or DPDK sleeping in a thread for 2 second.
Despite this limitation, vf commands may be executed from both
iavf_dev_alarm_handler() in a control thread and the applications main
thread. This is trivial to simulate in the testpmd application by
creating a bond of vf's in active backup mode, and then turning the
bond off and then on again repeatedly.
Previously [1] was proposed as a potential solution, but this commit did
not resolve all potential issues concerning the admin queue and has been
reverted from the stable branch. I propose adding locks until a more
complete solution is available.
[1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")
Fixes: 48de41ca11f0 ("net/avf: enable link status update")
Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
Cc: sta...@dpdk.org
Signed-off-by: Mike Pattrick
---
v2:
- Added locks around some iavf_execute_vf_cmd calls which were missed
in v1
- Changed description to indicate that [1] was only reverted out of the
stable branch.
v3:
- Refacted all locks into single function.
---
drivers/net/iavf/iavf.h | 1 +
drivers/net/iavf/iavf_vchnl.c | 105 --
2 files changed, 62 insertions(+), 44 deletions(-)
diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h
index 1edebab8dc..aa18650ffa 100644
--- a/drivers/net/iavf/iavf.h
+++ b/drivers/net/iavf/iavf.h
@@ -262,6 +262,7 @@ struct iavf_info {
struct iavf_qv_map *qv_map; /* queue vector mapping */
struct iavf_flow_list flow_list;
rte_spinlock_t flow_ops_lock;
+ rte_spinlock_t aq_lock;
struct iavf_parser_list rss_parser_list;
struct iavf_parser_list dist_parser_list;
struct iavf_parser_list ipsec_crypto_parser_list;
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index f92daf97f2..7bd992c882 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -397,6 +397,19 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct
iavf_cmd_info *args,
return err;
}
+static int
+iavf_execute_vf_cmd_safe(struct iavf_adapter *adapter,
+ struct iavf_cmd_info *args, int async)
+{
+ int ret;
+
+ rte_spinlock_lock(&vf->aq_lock);
+ ret = iavf_execute_vf_cmd(adapter, args, async);
+ rte_spinlock_unlock(&vf->aq_lock);
+
+ return ret;
+}
+
static void
iavf_handle_pf_event_msg(struct rte_eth_dev *dev, uint8_t *msg,
uint16_t msglen)
@@ -554,7 +567,7 @@ iavf_enable_vlan_strip(struct iavf_adapter *adapter)
args.in_args_size = 0;
args.out_buffer = vf->aq_resp;
args.out_size = IAVF_AQ_BUF_SZ;
- ret = iavf_execute_vf_cmd(adapter, &args, 0);
+ ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (ret)
PMD_DRV_LOG(ERR, "Failed to execute command of"
" OP_ENABLE_VLAN_STRIPPING");
@@ -575,7 +588,7 @@ iavf_disable_vlan_strip(struct iavf_adapter *adapter)
args.in_args_size = 0;
args.out_buffer = vf->aq_resp;
args.out_size = IAVF_AQ_BUF_SZ;
- ret = iavf_execute_vf_cmd(adapter, &args, 0);
+ ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (ret)
PMD_DRV_LOG(ERR, "Failed to execute command of"
" OP_DISABLE_VLAN_STRIPPING");
@@ -604,7 +617,7 @@ iavf_check_api_version(struct iavf_adapter *adapter)
args.out_buffer = vf->aq_resp;
args.out_size = IAVF_AQ_BUF_SZ;
- err = iavf_execute_vf_cmd(adapter, &args, 0);
+ err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (err) {
PMD_INIT_LOG(ERR, "Fail to execute command of OP_VERSION");
return err;
@@ -665,7 +678,7 @@ iavf_get_vf_resource(struct iavf_adapter *adapter)
args.in_args = (uint8_t *)∩︀
args.in_args_size = sizeof(caps);
- err = iavf_execute_vf_cmd(adapter, &args, 0);
+ err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (err) {
PMD_DRV_LOG(ERR,
@@ -710,7 +723,7 @@ iavf_get_supported_rxdid(struct iavf_adapter *adapter)
args.out_buffer = vf->aq_resp;
args.out_size = IAVF_AQ_BUF_SZ;
- ret = iavf_execute_vf_cmd(adapter, &args, 0);
+ ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (ret) {
PMD_DRV_LOG(ERR,
"Failed to execute command of
OP_GET_SUPPORTED_RXDIDS");
@@ -754,7 +767,7 @@ iavf_config_vlan_strip_v2(struct iavf_adapter *adapter,
bool enable)
args.in_args_size = sizeof(vlan_strip);
args.out_buffer = vf->aq_resp;
args.out_size = IAVF_AQ_BUF_SZ;
- ret = iavf_execute_vf_cmd(adapter, &args, 0);
+
Get started contributing to the DPDK
Dear DPDK team, My name is Raul and I recently discovered the DPDK project and I am very interested in contributing and becoming more involved. Can you please provide some guidance on how to get started with my first contribution? I am particularly interested in tackling bugs that are suitable for beginners. Thank you for your help. Sincerely, Raul PS: I do not know where to post this, let me know if this mailing list is not the suitable method.
[PATCH v4] net/iavf: add lock for vf commands
iavf admin queue commands aren't thread-safe. Bugs surrounding this
issue can manifest in a variety of ways but frequently pend_cmd is
over written. Simultaneously executing commands can result in a
misconfigured device or DPDK sleeping in a thread for 2 second.
Despite this limitation, vf commands may be executed from both
iavf_dev_alarm_handler() in a control thread and the applications main
thread. This is trivial to simulate in the testpmd application by
creating a bond of vf's in active backup mode, and then turning the
bond off and then on again repeatedly.
Previously [1] was proposed as a potential solution, but this commit did
not resolve all potential issues concerning the admin queue and has been
reverted from the stable branch. I propose adding locks until a more
complete solution is available.
[1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")
Fixes: 48de41ca11f0 ("net/avf: enable link status update")
Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
Cc: sta...@dpdk.org
Signed-off-by: Mike Pattrick
---
v2:
- Added locks around some iavf_execute_vf_cmd calls which were missed
in v1
- Changed description to indicate that [1] was only reverted out of the
stable branch.
v3:
- Refacted all locks into single function.
v4:
- Previous patch got corrupted
---
drivers/net/iavf/iavf.h | 1 +
drivers/net/iavf/iavf_vchnl.c | 106 --
2 files changed, 63 insertions(+), 44 deletions(-)
diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h
index 1edebab8dc..aa18650ffa 100644
--- a/drivers/net/iavf/iavf.h
+++ b/drivers/net/iavf/iavf.h
@@ -262,6 +262,7 @@ struct iavf_info {
struct iavf_qv_map *qv_map; /* queue vector mapping */
struct iavf_flow_list flow_list;
rte_spinlock_t flow_ops_lock;
+ rte_spinlock_t aq_lock;
struct iavf_parser_list rss_parser_list;
struct iavf_parser_list dist_parser_list;
struct iavf_parser_list ipsec_crypto_parser_list;
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index f92daf97f2..9adaadb173 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -397,6 +397,20 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct
iavf_cmd_info *args,
return err;
}
+static int
+iavf_execute_vf_cmd_safe(struct iavf_adapter *adapter,
+ struct iavf_cmd_info *args, int async)
+{
+ struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
+ int ret;
+
+ rte_spinlock_lock(&vf->aq_lock);
+ ret = iavf_execute_vf_cmd(adapter, args, async);
+ rte_spinlock_unlock(&vf->aq_lock);
+
+ return ret;
+}
+
static void
iavf_handle_pf_event_msg(struct rte_eth_dev *dev, uint8_t *msg,
uint16_t msglen)
@@ -554,7 +568,7 @@ iavf_enable_vlan_strip(struct iavf_adapter *adapter)
args.in_args_size = 0;
args.out_buffer = vf->aq_resp;
args.out_size = IAVF_AQ_BUF_SZ;
- ret = iavf_execute_vf_cmd(adapter, &args, 0);
+ ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (ret)
PMD_DRV_LOG(ERR, "Failed to execute command of"
" OP_ENABLE_VLAN_STRIPPING");
@@ -575,7 +589,7 @@ iavf_disable_vlan_strip(struct iavf_adapter *adapter)
args.in_args_size = 0;
args.out_buffer = vf->aq_resp;
args.out_size = IAVF_AQ_BUF_SZ;
- ret = iavf_execute_vf_cmd(adapter, &args, 0);
+ ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (ret)
PMD_DRV_LOG(ERR, "Failed to execute command of"
" OP_DISABLE_VLAN_STRIPPING");
@@ -604,7 +618,7 @@ iavf_check_api_version(struct iavf_adapter *adapter)
args.out_buffer = vf->aq_resp;
args.out_size = IAVF_AQ_BUF_SZ;
- err = iavf_execute_vf_cmd(adapter, &args, 0);
+ err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (err) {
PMD_INIT_LOG(ERR, "Fail to execute command of OP_VERSION");
return err;
@@ -665,7 +679,7 @@ iavf_get_vf_resource(struct iavf_adapter *adapter)
args.in_args = (uint8_t *)∩︀
args.in_args_size = sizeof(caps);
- err = iavf_execute_vf_cmd(adapter, &args, 0);
+ err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (err) {
PMD_DRV_LOG(ERR,
@@ -710,7 +724,7 @@ iavf_get_supported_rxdid(struct iavf_adapter *adapter)
args.out_buffer = vf->aq_resp;
args.out_size = IAVF_AQ_BUF_SZ;
- ret = iavf_execute_vf_cmd(adapter, &args, 0);
+ ret = iavf_execute_vf_cmd_safe(adapter, &args, 0);
if (ret) {
PMD_DRV_LOG(ERR,
"Failed to execute command of
OP_GET_SUPPORTED_RXDIDS");
@@ -754,7 +768,7 @@ iavf_config_vlan_strip_v2(struct iavf_adapter *adapter,
bool enable)
args.in_args_size = sizeof(vlan_strip);
args.out_buffer = vf->aq_resp;
