RE: [PATCH v2] dts: Change hugepage runtime config to 2MB Exclusively
> From: Nicholas Pratte [mailto:npra...@iol.unh.edu]
> Sent: Tuesday, 9 April 2024 19.28
>
> The previous implementation configures and allocates hugepage sizes
> based on a system default. This can lead to two problems: overallocation of
> hugepages (which may crash the remote host), and configuration of hugepage
> sizes that are not recommended during runtime. This new implementation
> allows only 2MB hugepage allocation during runtime; any other unique
> hugepage size must be configured by the end-user for initializing DTS.
>
> If the amount of 2MB hugepages requested exceeds the amount of 2MB
> hugepages already configured on the system, then the system will remount
> hugepages to cover the difference. If the amount of hugepages requested is
> either less than or equal to the amount already configured on the system,
> then nothing is done.
>
> Bugzilla ID: 1370
> Signed-off-by: Nicholas Pratte
> Reviewed-by: Jeremy Spewock
> ---
> dts/conf.yaml| 4 ++--
> dts/framework/config/__init__.py | 4 ++--
> dts/framework/config/conf_yaml_schema.json | 6 +++---
> dts/framework/config/types.py| 2 +-
> dts/framework/testbed_model/linux_session.py | 15 ++-
> 5 files changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/dts/conf.yaml b/dts/conf.yaml
> index 8068345dd5..56c3ae6f4c 100644
> --- a/dts/conf.yaml
> +++ b/dts/conf.yaml
> @@ -35,7 +35,7 @@ nodes:
> lcores: "" # use all the available logical cores
> use_first_core: false # tells DPDK to use any physical core
> memory_channels: 4 # tells DPDK to use 4 memory channels
> -hugepages: # optional; if removed, will use system hugepage
> configuration
> +hugepages_2mb: # optional; if removed, will use system hugepage
> configuration
> amount: 256
> force_first_numa: false
> ports:
> @@ -71,7 +71,7 @@ nodes:
> os_driver: rdma
> peer_node: "SUT 1"
> peer_pci: ":00:08.1"
> -hugepages: # optional; if removed, will use system hugepage
> configuration
> +hugepages_2mb: # optional; if removed, will use system hugepage
> configuration
> amount: 256
> force_first_numa: false
> traffic_generator:
> diff --git a/dts/framework/config/__init__.py
> b/dts/framework/config/__init__.py
> index 4cb5c74059..b6f820e39e 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -255,8 +255,8 @@ def from_dict(
> Either an SUT or TG configuration instance.
> """
> hugepage_config = None
> -if "hugepages" in d:
> -hugepage_config_dict = d["hugepages"]
> +if "hugepages_2mb" in d:
> +hugepage_config_dict = d["hugepages_2mb"]
> if "force_first_numa" not in hugepage_config_dict:
> hugepage_config_dict["force_first_numa"] = False
> hugepage_config = HugepageConfiguration(**hugepage_config_dict)
> diff --git a/dts/framework/config/conf_yaml_schema.json
> b/dts/framework/config/conf_yaml_schema.json
> index 4731f4511d..f4d7199523 100644
> --- a/dts/framework/config/conf_yaml_schema.json
> +++ b/dts/framework/config/conf_yaml_schema.json
> @@ -146,7 +146,7 @@
> "compiler"
>]
> },
> -"hugepages": {
> +"hugepages_2mb": {
>"type": "object",
>"description": "Optional hugepage configuration. If not specified,
> hugepages won't be configured and DTS will use system configuration.",
>"properties": {
> @@ -253,8 +253,8 @@
> "type": "integer",
> "description": "How many memory channels to use. Optional,
> defaults to 1."
>},
> - "hugepages": {
> -"$ref": "#/definitions/hugepages"
> + "hugepages_2mb": {
> +"$ref": "#/definitions/hugepages_2mb"
>},
>"ports": {
> "type": "array",
> diff --git a/dts/framework/config/types.py b/dts/framework/config/types.py
> index 1927910d88..016e0c3dbd 100644
> --- a/dts/framework/config/types.py
> +++ b/dts/framework/config/types.py
> @@ -46,7 +46,7 @@ class NodeConfigDict(TypedDict):
> """Allowed keys and values."""
>
> #:
> -hugepages: HugepageConfigurationDict
> +hugepages_2mb: HugepageConfigurationDict
> #:
> name: str
> #:
> diff --git a/dts/framework/testbed_model/linux_session.py
> b/dts/framework/testbed_model/linux_session.py
> index 5d24030c3d..37f5eacb21 100644
> --- a/dts/framework/testbed_model/linux_session.py
> +++ b/dts/framework/testbed_model/linux_session.py
> @@ -15,7 +15,7 @@
>
> from typing_extensions import NotRequired
>
> -from framework.exception import RemoteCommandExecutionError
> +from framework.exception import ConfigurationError,
> RemoteCommandExecutionError
> from framework.utils import expand_range
>
> from .cpu import LogicalCore
> @@ -87,25 +87,22 @@ def get_dpdk_file_prefix(self, dpdk
Re: [PATCH 0/4] RFC samples converting VLA to alloca
On 2024-04-08 17:27, Tyler Retzlaff wrote: For next technboard meeting. On Sun, Apr 07, 2024 at 10:03:06AM -0700, Stephen Hemminger wrote: On Sun, 7 Apr 2024 13:07:06 +0200 Morten Brørup wrote: From: Mattias Rönnblom [mailto:hof...@lysator.liu.se] Sent: Sunday, 7 April 2024 11.32 On 2024-04-04 19:15, Tyler Retzlaff wrote: This series is not intended for merge. It insteat provides examples of converting use of VLAs to alloca() would look like. what's the advantages of VLA over alloca()? * sizeof(array) works as expected. * multi-dimensional arrays are still arrays instead of pointers to dynamically allocated space. this means multiple subscript syntax works (unlike on a pointer) and calculation of addresses into allocated space in ascending order is performed by the compiler instead of manually. alloca() is a pretty obscure mechanism, and also not a part of the C standard. VLAs are C99, and well-known and understood, and very efficient. The RFC fails to mention why we need to replace VLAs with something else: VLAs are C99, but not C++; VLAs were made optional in C11. MSVC doesn't support VLAs, and is not going to: https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/#variable-length-arrays I dislike alloca() too, and the notes section in the alloca(3) man page even discourages the use of alloca(): https://man7.org/linux/man-pages/man3/alloca.3.html But I guess alloca() is the simplest replacement for VLAs. This RFC patch series opens the discussion for alternatives in different use cases. The other issue with VLA's is that if the number is something that can be externally input, then it can be a source of stack overflow bugs. That is why the Linux kernel has stopped using them; for security reasons. DPDK has much less of a security trust domain. Mostly need to make sure that no data from network is being used to compute VLA size. Looks like we need to discuss this at the next techboard meeting. * MSVC doesn't support C11 optional VLAs (and never will). This is due to dogmatism, or what? Surely, a lot of Open Source projects written for C99 will use VLAs. * alloca() is an alternative that is available on all platforms/toolchain combinations. alloca() is a poor alternative. The use of alloca() should be restricted to situations where statically sized arrays can't do the job. * it's reasonable for some VLAs to be turned into regular arrays but it would be unsatisfactory to be stuck waiting discussions of defining new constant expression macros on a per-use basis. * there is resistance to using alloca() vs VLA so my proposal is to change only the code that is built to target windows.
Re: [PATCH 0/4] RFC samples converting VLA to alloca
On 2024-04-08 17:53, Morten Brørup wrote: From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] Sent: Monday, 8 April 2024 17.27 For next technboard meeting. On Sun, Apr 07, 2024 at 10:03:06AM -0700, Stephen Hemminger wrote: On Sun, 7 Apr 2024 13:07:06 +0200 Morten Brørup wrote: From: Mattias Rönnblom [mailto:hof...@lysator.liu.se] Sent: Sunday, 7 April 2024 11.32 On 2024-04-04 19:15, Tyler Retzlaff wrote: This series is not intended for merge. It insteat provides examples of converting use of VLAs to alloca() would look like. what's the advantages of VLA over alloca()? * sizeof(array) works as expected. * multi-dimensional arrays are still arrays instead of pointers to dynamically allocated space. this means multiple subscript syntax works (unlike on a pointer) and calculation of addresses into allocated space in ascending order is performed by the compiler instead of manually. alloca() is a pretty obscure mechanism, and also not a part of the C standard. VLAs are C99, and well-known and understood, and very efficient. The RFC fails to mention why we need to replace VLAs with something else: VLAs are C99, but not C++; VLAs were made optional in C11. MSVC doesn't support VLAs, and is not going to: https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support- arriving-in-msvc/#variable-length-arrays I dislike alloca() too, and the notes section in the alloca(3) man page even discourages the use of alloca(): https://man7.org/linux/man-pages/man3/alloca.3.html But I guess alloca() is the simplest replacement for VLAs. This RFC patch series opens the discussion for alternatives in different use cases. The other issue with VLA's is that if the number is something that can be externally input, then it can be a source of stack overflow bugs. That is why the Linux kernel has stopped using them; for security reasons. DPDK has much less of a security trust domain. Mostly need to make sure that no data from network is being used to compute VLA size. Looks like we need to discuss this at the next techboard meeting. * MSVC doesn't support C11 optional VLAs (and never will). * alloca() is an alternative that is available on all platforms/toolchain combinations. * it's reasonable for some VLAs to be turned into regular arrays but it would be unsatisfactory to be stuck waiting discussions of defining new constant expression macros on a per-use basis. We must generally stop using VLAs, for many reasons. What reasons would that be? And which of those reasons are not also reasons to stop using alloca(). The only available 1:1 replacement is alloca(), so we have to accept that. If anyone still cares about improvements, we can turn alloca()'d arrays into regular arrays after this patch series. Alternatives to VLAs are very interesting discussions, but let's not stall MSVC progress because of it! What is this supposed to mean? Finding alternatives to VLAs are required to make progress of MSVC support in DPDK. * there is resistance to using alloca() vs VLA so my proposal is to change only the code that is built to target windows. I would prefer to get rid of them all, so the CI can build with -Wvla to prevent them from being introduced again. Not a strong preference. On the other hand, the CI's MSVC builds will catch them if used for a Windows target. And limiting to Windows code reduces the amount of work, so that's probably the most realistic solution.
Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
On Thu, Mar 28, 2024 at 5:49 PM Jeremy Spewock wrote: > > On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro wrote: > > > > This commit provides a state container for TestPmdShell. It currently > > only indicates whether the packet forwarding has started > > or not, and the number of ports which were given to the shell. > > > > This also fixes the behaviour of `wait_link_status_up` to use the > > command timeout as inherited from InteractiveShell. > > > > Signed-off-by: Luca Vizzarro > > Reviewed-by: Jack Bond-Preston > > Reviewed-by: Honnappa Nagarahalli > > --- > > > @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: > > Callable[[str], str] | None > > if self._app_args.app_params is None: > > self._app_args.app_params = TestPmdParameters() > > > > -self.number_of_ports = len(self._app_args.ports) if > > self._app_args.ports is not None else 0 > > +assert isinstance(self._app_args.app_params, TestPmdParameters) > > + > > This is tricky because ideally we wouldn't have the assertion here, > but I understand why it is needed because Eal parameters have app args > which can be any instance of params. I'm not sure of the best way to > solve this, because making testpmd parameters extend from eal would > break the general scheme that you have in place, and having an > extension of EalParameters that enforces this app_args is > TestPmdParameters would solve the issues, but might be a little > clunky. Is there a way we can use a generic to get python to just > understand that, in this case, this will always be TestPmdParameters? > If not I might prefer making a private class where this is > TestPmdParameters, just because there aren't really any other > assertions that we use elsewhere and an unexpected exception from this > (even though I don't think that can happen) could cause people some > issues. > > It might be the case that an assertion is the easiest way to deal with > it though, what do you think? > We could change the signature (just the type of app_args) of the init method - I think we should be able to create a type that's EalParameters with .app_params being TestPmdParameters or None. The init method would just call super(). Something like the above is basically necessary with inheritance where subclasses are all extensions (not just implementations) of the superclass (having differences in API). > > +if self._app_args.app_params.auto_start: > > +self.state.packet_forwarding_started = True > > + > > +if self._app_args.ports is not None: > > +self.state.number_of_ports = len(self._app_args.ports) > > > > super()._start_application(get_privileged_command) > > > > > 2.34.1 > >
Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro wrote:
>
> This commit provides a state container for TestPmdShell. It currently
> only indicates whether the packet forwarding has started
> or not, and the number of ports which were given to the shell.
>
A reminder, the commit message should explain why we're doing this
change, not what the change is.
> This also fixes the behaviour of `wait_link_status_up` to use the
> command timeout as inherited from InteractiveShell.
>
> Signed-off-by: Luca Vizzarro
> Reviewed-by: Jack Bond-Preston
> Reviewed-by: Honnappa Nagarahalli
> ---
> dts/framework/remote_session/testpmd_shell.py | 41 +--
> 1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py
> b/dts/framework/remote_session/testpmd_shell.py
> index a823dc53be..ea1d254f86 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -678,19 +678,27 @@ def __str__(self) -> str:
> return self.pci_address
>
>
> +@dataclass(slots=True)
> +class TestPmdState:
> +"""Session state container."""
> +
> +#:
> +packet_forwarding_started: bool = False
The same question as in the previous patch, do you anticipate this
being needed and should we add this only when it's actually used?
> +
> +#: The number of ports which were allowed on the command-line when
> testpmd was started.
> +number_of_ports: int = 0
> +
> +
> class TestPmdShell(InteractiveShell):
> """Testpmd interactive shell.
>
> The testpmd shell users should never use
> the :meth:`~.interactive_shell.InteractiveShell.send_command` method
> directly, but rather
> call specialized methods. If there isn't one that satisfies a need, it
> should be added.
> -
> -Attributes:
> -number_of_ports: The number of ports which were allowed on the
> command-line when testpmd
> -was started.
> """
>
> -number_of_ports: int
> +#: Current state
> +state: TestPmdState = TestPmdState()
Assigning a value makes this a class variable, shared across all
instances. This should be initialized in __init__().
But do we actually want to do this via composition? We'd need to
access the attributes via .state all the time and I don't really like
that. We could just put them into TestPmdShell directly, initializing
them in __init__().
>
> #: The path to the testpmd executable.
> path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")
RE: [PATCH 0/4] RFC samples converting VLA to alloca
> From: Mattias Rönnblom [mailto:hof...@lysator.liu.se] > Sent: Wednesday, 10 April 2024 09.32 > > On 2024-04-08 17:53, Morten Brørup wrote: > >> From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > >> Sent: Monday, 8 April 2024 17.27 > >> [...] > >> Looks like we need to discuss this at the next techboard meeting. > >> > >> * MSVC doesn't support C11 optional VLAs (and never will). > >> * alloca() is an alternative that is available on all platforms/toolchain > >>combinations. > >> * it's reasonable for some VLAs to be turned into regular arrays but it > >>would be unsatisfactory to be stuck waiting discussions of defining new > >>constant expression macros on a per-use basis. > > > > We must generally stop using VLAs, for many reasons. > > What reasons would that be? And which of those reasons are not also > reasons to stop using alloca(). The reasons against VLAs are the same as why MSVC doesn’t support them; primarily that they are insecure. The reasons against VLAs and alloca() are the same, except MSVC supports alloca(). > > > The only available 1:1 replacement is alloca(), so we have to accept that. > > > > If anyone still cares about improvements, we can turn alloca()'d arrays into > regular arrays after this patch series. > > > > Alternatives to VLAs are very interesting discussions, but let's not stall > MSVC progress because of it! > > > > What is this supposed to mean? Finding alternatives to VLAs are required > to make progress of MSVC support in DPDK. It means that not enough people contribute to discussing and implementing alternatives, so we have to use the 1:1 replacement alternative, alloca(), to avoid stalling DPDK support for MSVC. We can discuss and implement alternatives at any time, if anybody cares. > > >> * there is resistance to using alloca() vs VLA so my proposal is to > >>change only the code that is built to target windows. > > > > I would prefer to get rid of them all, so the CI can build with -Wvla to > prevent them from being introduced again. > > Not a strong preference. > > On the other hand, the CI's MSVC builds will catch them if used for a > Windows target. > > And limiting to Windows code reduces the amount of work, so that's probably > the most realistic solution. > >
Re: [PATCH 1/6] dts: add parameters data structure
On Tue, Apr 9, 2024 at 6:28 PM Luca Vizzarro wrote:
>
> Thank you so much for your review Juraj!
>
You're welcome!
> >> +
> >> +def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> >> +"""Injects the value of the attribute as-is without flag. Metadata
> >> modifier for :func:`dataclasses.field`."""
> >> +return {**metadata, META_VALUE_ONLY: True}
> >
> > These methods, on the other hand, are used outside this module, so it
> > makes sense to have them outside Params. It could be better to have
> > them inside as static methods, as they're closely related. Looking at
> > how they're used, we should unite the imports:
> > 1. In testpmd_shell, they're imported directly (from framework.params
> > import long)
> > 2. In sut_node, just the params module is imported (from framework
> > import params and then accessed via it: metadata=params.short("l"))
> >
> Having a shorter version may look less verbose. I agree that we can make
> everything a static method of Params, but then having to do Params.short
> etc everytime will make it look more verbose. So what option do we
> prefer? The functions do belong to the params module nonetheless, and
> therefore are meant to be used in conjunction with the Params class.
>
When I first saw the code, I liked the usage in sut_node better, e.g.:
prefix: str = field(metadata=params.long("file-prefix")). I think this
is because it's obvious where the function comes from. I'd do the
longer version because I think most people are just going to glance at
the code when they want to know how to properly implement an argument
so the explicit nature could help with understanding how it should be
done.
> > If we move these to Params, we could import Params and use them
> > similarly to 2. Not sure which one is better.
> >
>
>
> >> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) ->
> >> dict[str, Any]:
> >
> > Any reason why mixins are plural? I've only seen this used with one
> > argument, do you anticipate we'd need to use more than one? We could
> > make this singular and if we ever need to do two things, we could just
> > pass a function that does those two things (or calls two different
> > functions). Also, I'd just rename the mixin the func or something like
> > that.
> >
> > The default of an argument should not be mutable, here's a quick
> > explanation:
> > https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
>
>
> Indeed the reason for which I create dictionaries, as I am treating them
> as read only. I wanted to avoid to bloat the code with lots of `is None`
> checks. But we can sacrifice this optimization for better code.
>
This would be the only place where we'd do the check, as I don't think
we need the metadata argument in any of the other functions - those
seem to be mutually exclusive, but maybe they're not? In any case,
we'd need to fix this, I don't think treating them as read-only avoids
the problem.
> > I don't really like the name. The positional arguments are applied to
> > the value and that should be reflected in the name - I'd like to see
> > something like convert in the name.
>
> What this does is effectively just add the mixins to dataclass.field
> metadata, hence "field"_mixins. This is meant to represent a chain of
> mixins, in my original code this appeared more often. Not so much in
> this post, as I made more optimisations. Which takes me to the plural
> bit, for testpmd specifically this plurality appears only when
> decorating another class using `str_mixins`, see TestPmdRingNUMAConfig.
> So for consistency I kept both to ingest a chain of "mixins", as maybe
> it'll be needed in the future.
>
> Are you suggesting to just make the name as singular? But still take
> multiple function pointers?
Singular with one function, as that was what I saw being used. The one
function could do multiple things (or call multiple other functions)
if a need arises. The str_mixins could be used this way as well.
I don't know which one is better, maybe keeping the plural is fine.
> The term "mixin" specifically just means a
> middleware that manipulates the output value when using __str__.
Aren't all of the other functions mixins as well, at least in some
sense? They change the option, not the value, but could still be
thought of as mixins in some respect.
> Here we
> are just chaining them for reusability. Do you have any better name in mind?
>
I don't know, so let's brainstorm a bit. Let's start with the usage:
portmask: int | None = field(default=None, metadata=field_mixins(hex))
Here it's not clear at all why it's called field_mixins, at least
compared to the other functions which are not called mixins. I guess
the other functions are predefined option mixins whereas we're
supplying our own value mixins here. I also noticed that there's a bit
of an inconsistency with the naming. The basic functions (long etc.)
don't have the "field_" prefix, but this one does. Maybe a better name
would be cus
Strict aliasing problem with rte_eth_linkstatus_set()
Hi All,
We have a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
we've done some
research but haven't been able to figure out why. We'd like the community's
help.
Environment:
1. Source: DPDK 23.11
2. GCC: 12.3.1 [1]
3. Compiled with target kunpeng SoC (ARM64)
4. Run on kunpeng SoC
Problem & Debug:
1. We found the hns3 driver fails to update the link status. The corresponding
function is
hns3_update_linkstatus_and_event [2], and we found the rte_eth_linkstatus_set
[3] always return zero.
2. After disassembly the hns3_update_linkstatus_and_event, and found
rte_eth_linkstatus_set's
rte_atomic_exchange_explicit return to xzr register (which is zero register):
1239fec: 3900f3e0strbw0, [sp, #60]
1239ff0: 9101a041add x1, x2, #0x68 ---x2 seem not the
variable new_link
1239ff4: f8ff8022swpal xzr, x2, [x1] ---this instr
corresponding rte_atomic_exchange_explicit,
it will place the
resut in xzr which always zero,
and this will
lead to rte_eth_linkstatus_set return 0.
1239ff8: 3940f3e0ldrbw0, [sp, #60]
1239ffc: d3609c41ubfxx1, x2, #32, #8
123a000: 4a01eor w0, w0, w1
123a004: 36100080tbz w0, #2, 123a014
3. We checked other "ret = rte_eth_linkstatus_set" calls, and can't find
similar problem.
4. After reading a lot of documents, we preliminarily think that the problem is
caused by -fstrict-aliasing
(which was enabled default with O2 or O3), if compiled with
-fno-strict-aliasing, then this problem don't
exist. We guest this maybe strict-aliasing's bug which only happened in our
function.
5. We also try to use union to avoid such aliasing in rte_eth_linkstatus_set,
we changed the struct
rte_eth_link define, and it works:
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write
*/
- uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
- uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
- uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
- uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link { /**< aligned for atomic64 read/write */
+ union {
+ uint64_t val64;
+ struct {
+ uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
+ uint16_t link_duplex : 1; /**<
RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+ uint16_t link_autoneg : 1; /**<
RTE_ETH_LINK_[AUTONEG/FIXED] */
+ uint16_t link_status : 1; /**<
RTE_ETH_LINK_[DOWN/UP] */
+ };
+ };
};
the corresponding rte_eth_linkstatus_set:
@@ -1674,18 +1674,13 @@ static inline int
rte_eth_linkstatus_set(struct rte_eth_dev *dev,
const struct rte_eth_link *new_link)
{
- RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic
*)&(dev->data->dev_link);
- union {
- uint64_t val64;
- struct rte_eth_link link;
- } orig;
-
- RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+ struct rte_eth_link old_link;
- orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t
*)new_link,
+ old_link.val64 =
rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+ new_link->val64,
rte_memory_order_seq_cst);
- return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+ return (old_link.link_status == new_link->link_status) ? -1 : 0;
}
6. BTW: the linux kernel enabled "-fno-strict-aliasing" default, please see [4]
for more.
Last： We think there are two ways to solve this problem.
1. Add the compilation option '-fno-strict-aliasing' for hold DPDK project.
2. Use union to avoid such aliasing in rte_eth_linkstatus_set (please see
above).
PS: We prefer first way.
Hope for more discuess.
Thanks
[1] https://developer.arm.com/downloads/-/arm-gnu-toolchain-downloads/12-3-rel1
[2] void
hns3_update_linkstatus_and_event(struct hns3_hw *hw, bool query)
{
struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
struct rte_eth_link new_link;
int ret;
if (query)
hns3_update_port_link_info(dev);
memset(&new_link, 0, sizeof(new_link));
hns3_setup_linkstatus(dev, &new_link);
ret = rte_eth_linkstatus_set(dev, &new_link);
if (ret == 0 && dev->data->dev_conf.intr_conf.lsc != 0)
hns3_start_report_lse(dev);
}
[3] static inline int
rte_eth_linkstatus_set(struct rte_eth_dev *dev,
const struct rte_eth_link *new_link)
{
RTE_ATOMIC(uint64_t) *dev_l
Re: [PATCH 2/6] dts: use Params for interactive shells
On 09/04/2024 15:56, Juraj Linkeš wrote: On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock wrote: I'm not sure if allowing None should be the solution for these shells as opposed to just supplying an empty parameter object. Maybe something that could be done is the factory method in sut_node allows it to be None, but when it comes time to make the abstract shell it creates an empty one if it doesn't exist, or the init method makes this an optional parameter but creates it if it doesn't exist. I suppose this logic would have to exist somewhere because the parameters aren't always required, it's just a question of where we should put it and if we should just assume that the interactive shell class just knows how to accept some parameters and put them into the shell. I would maybe leave this as something that cannot be None and handle it outside of the shell, but I'm not sure it's something really required and I don't have a super strong opinion on it. I think this is an excellent idea. An empty Params (or StrParams or EalParams if we want to make Params truly abstract and thus not instantiable) would work as the default value and it would be an elegant solution since dev will only pass non-empty Params. I left it as generic as it could get as I honestly couldn't grasp the full picture. I am really keen to ditch everything else and use an empty Params object instead for defaults. And as Juraj said, if I am making Params a true abstract object, then it'd be picking one of the Params subclasses mentioned above. I guess EalParams could only be used with shells are that sure to be DPDK apps, and I presume that's only TestPmdShell for now. +from framework.testbed_model.sut_node import EalParameters + +assert isinstance(self._app_args, EalParameters) + +if isinstance(self._app_args.app_params, StrParams): +self._app_args.app_params.value += " -i --mask-event intr_lsc" + +self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0 I we should override the _app_args parameter in the testpmd shell to always be EalParameters instead of doing this import and assertion. It's a DPDK app, so we will always need EalParameters anyway, so we might as well put that as our typehint to start off as what we expect instead. The checking of an instance of StrParams also feels a little strange here, it might be more ideal if we could just add the parameters without this check. Maybe something we can do, just because these parameters are meant to be CLI commands anyway and will be rendered as a string, is replace the StrParams object with a method on the base Params dataclass that allows you to just add any string as a value only field. Then, we don't have to bother validating anything about the app parameters and we don't care what they are, we can just add a string to them of new parameters. I think this is something that likely also gets solved when you replace this with testpmd parameters, but it might be a good way to make the Params object more versatile in general so that people can diverge and add their own flags to it if needed. Jeremy, I agree this is not ideal. Although this was meant only to be transitionary for the next commit (as you say it gets resolved with TestPmdParams). But I agree with you that we can integrate the StrParams facility within Params. This means no longer making Params a true abstract class though, which is something I can live with, especially if we can make it simpler. I'd say this is done because of circular imports. If so, we could move EalParameters to params.py, that should help. And when we're at it, either rename it to EalParams or rename the other classes to use the whole word. Yeah, the circular imports are the main problem indeed. I considered refactoring but noticed it'd take more than just moving EalParam(eter)s to params.py. Nonetheless, keen to make a bigger refactoring to make this happen. + super()._start_application(get_privileged_command) def start(self, verify: bool = True) -> None: @@ -134,7 +136,7 @@ def create_interactive_shell( shell_cls: Type[InteractiveShellType], timeout: float, privileged: bool, -app_args: str, +app_args: Params | None, This also falls in line with what I was saying before about where this logic is handled. This was made to always be a string originally because by this point it being optional was already handled by the sut_node.create_interactive_shell() and we should have some kind of value here (even if that value is an empty parameters dataclass) to pass into the application. It sort of semantically doesn't really change much, but at this point it might as well not be None, so we can simplify this type. Ack. ) -> InteractiveShellType: """Factory for interactive session handlers. +@dataclass(kw_only=True) +class EalParameters(Params): """The en
Re: [PATCH v2 1/3] dts: rework arguments framework
On Tue, Apr 9, 2024 at 5:14 PM Luca Vizzarro wrote: > > On 04/04/2024 10:25, Juraj Linkeš wrote: > > Judging from the code, this patch seems like a convoluted way to implement: > > 1. An association between an Argument and the corresponding > > environment variable, > > 2. A better way to add the env vars names to the help message of each > > argument as well as adding the current value if set, > > 3. A better error message where we replace argument names with env var > > names for args where env vars are used. > > > > But maybe there's more. > > > > In any case, isn't there a simpler way to implement the above? I > > originally thought extending something in the argparse module (like > > the add_argument methods in ArgumentParser and > > _MutuallyExclusiveGroup), but that doesn't seem as simple as maybe > > just augmenting the actions that are returned with the add_argument > > methods (maybe we could subclass ArgumentParser and add some method > > for this, such as add_dts_argument?), where we could just add the > > env_var_name (and maybe arg_name if needed) and modify the help > > message the same way we do now (modifying the self._action.help > > attribute). This should also be enough for 3, but even if not, we > > could store what we need in the subclass. > > I agree that the solutions appears somewhat convoluted, and I am indeed > open to ideas on how to simplify the approach. I initially considered > extending the argparse module, but as you said it's no particularly > simple, and it wasn't written to be particularly extendable either. If > we want to go down this route, it would be somewhat hacky. I am not > against it though. But, yeah, in retrospective some things can be easily > integrated. > Great, let's get a v2 and see where we end up. > > Also, what seems to be missing is the modification of actual values of > > SETTING with the env var values (this could be done somewhere in the > > add_dts_argument method). > > I don't think I am following what you mean by this. If you refer to > updating the values of `SETTING` with the environment ones, this is done > using `inject_env_variable` before the arguments are parsed. In a few > words, that method just injects the environment arguments in sys.argv. > Therefore to the ArgumentParser it just looks like they were supplied on > the command line. > My bad, I must've made a mistake when verifying this. It is working fine. > > But overall I like this approach as it gives us the knowledge of > > whether an env var was used or not. I have some comments that > > illustrate why I think the patch is a bit convoluted. > > > > > > > Looking at this, this class could've been just a subclassed dict. We > > could set the attributes with setattr in __init__(). But at that > > point, it looks to be the same as the namespace returned by > > parser.parse_args(), apart from the environment_fed_arguments property > > (more below), which we could do without. > > > > Yes, definitely. The main reason to store the namespaced values was in > case this could turn out to be useful when debugging in the future. But > if we think it's not worthwhile, it can reduce complexity. > > > > > We're already storing the arguments in the class, so we could just add > > whatever is in ArgumentEnvPair to the argument and we have the > > correspondence (looking at the Argument class again, we already have > > that). The pair class seems redundant. > > > > > > And then we could get all of this from the stored arguments. Could be > > just a tuple of (var_name, arg_name) of args with from_env == True. > > > > And storing a pre-made list of environment-fed arguments. Yes, we could > definitely walk through every argument as needed. Given the context and > usage of this, I guess yeah, you are right in saying it's redundant. > Happy to remove it. > > > > > I think this should also contain the env var value to be consistent > > with the help message. > > > > Ack. > > > > > We're going through all of the args here so we could just do this when > > creating the argument. I guess we'd need to modify the top-level error > > message afterwards with the current class structure, but I'm sure even > > that could be done when creating the argument with some refactoring. > > > > Yeah. I decided to do this separately to avoid duplicating the code for > the mutual exclusion group (as per the next commit). > > > > > The values attribute seems superfluous as we're only using it locally. > > > > Ack. > > > > > This is here so that we don't use both arguments from the mutual > > group, right? We could add a short comment explaining this. > > Also, I think we don't need to use vars() here, the result should be the > > same. > > > > Yes, and any other argument we may want to add in the future that don't > belong in SETTINGS. It's only selecting the arguments that already exist > in SETTING and write values for that. Can add a comment. > > Namespace doesn't appear to implement iteration. As per the doc pag
Re: [PATCH 1/6] dts: add parameters data structure
On 10/04/2024 10:15, Juraj Linkeš wrote:
+
+def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
+"""Injects the value of the attribute as-is without flag. Metadata modifier for
:func:`dataclasses.field`."""
+return {**metadata, META_VALUE_ONLY: True}
These methods, on the other hand, are used outside this module, so it
makes sense to have them outside Params. It could be better to have
them inside as static methods, as they're closely related. Looking at
how they're used, we should unite the imports:
1. In testpmd_shell, they're imported directly (from framework.params
import long)
2. In sut_node, just the params module is imported (from framework
import params and then accessed via it: metadata=params.short("l"))
Having a shorter version may look less verbose. I agree that we can make
everything a static method of Params, but then having to do Params.short
etc everytime will make it look more verbose. So what option do we
prefer? The functions do belong to the params module nonetheless, and
therefore are meant to be used in conjunction with the Params class.
When I first saw the code, I liked the usage in sut_node better, e.g.:
prefix: str = field(metadata=params.long("file-prefix")). I think this
is because it's obvious where the function comes from. I'd do the
longer version because I think most people are just going to glance at
the code when they want to know how to properly implement an argument
so the explicit nature could help with understanding how it should be
done.
Ack.
If we move these to Params, we could import Params and use them
similarly to 2. Not sure which one is better.
+def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) -> dict[str,
Any]:
Any reason why mixins are plural? I've only seen this used with one
argument, do you anticipate we'd need to use more than one? We could
make this singular and if we ever need to do two things, we could just
pass a function that does those two things (or calls two different
functions). Also, I'd just rename the mixin the func or something like
that.
The default of an argument should not be mutable, here's a quick
explanation:
https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
Indeed the reason for which I create dictionaries, as I am treating them
as read only. I wanted to avoid to bloat the code with lots of `is None`
checks. But we can sacrifice this optimization for better code.
This would be the only place where we'd do the check, as I don't think
we need the metadata argument in any of the other functions - those
seem to be mutually exclusive, but maybe they're not? In any case,
we'd need to fix this, I don't think treating them as read-only avoids
the problem.
They are not mutually exclusive. But thinking of it we can spare every
problem with having to chain "metadata" by letting the user do it
through the use of the pipe operator.
Here we
are just chaining them for reusability. Do you have any better name in mind?
I don't know, so let's brainstorm a bit. Let's start with the usage:
portmask: int | None = field(default=None, metadata=field_mixins(hex))
Here it's not clear at all why it's called field_mixins, at least
compared to the other functions which are not called mixins. I guess
the other functions are predefined option mixins whereas we're
supplying our own value mixins here. I also noticed that there's a bit
of an inconsistency with the naming. The basic functions (long etc.)
don't have the "field_" prefix, but this one does. Maybe a better name
would be custom_mixins? Or value_mixins? Or custom_value? Or maybe
convert_value? I like the last one:
portmask: int | None = field(default=None, metadata=convert_value(hex))
metadata=params.convert_value(_port_to_pci,
metadata=params.multiple(params.short("a"))), # in sut_node
I think this is easier to grasp. I'm thinking about whether we need to
have mixin(s) in the name and I don't think it adds much. If I'm a
developer, I'm looking at these functions and I stumble upon
convert_value, what I'm thinking is "Nice, I can do some conversion on
the values I pass, how do I do that?", then I look at the signature
and find out that I expect, that is I need to pass a function (or
multiple function if I want to). I guess this comes down to the
function name (field_mixins) not conveying what it's doing, rather
what you're passing to it.
So my conclusion from this brainstorming is that a better name would
be convert_value. :-)
Also, unrelated, but the context is lost. Another thing I just noticed
is in the docstring:
The ``metadata`` keyword argument can be used to chain metadata
modifiers together.
We're missing the Args: section in all of the docstrings (where we
could put the above). Also the Returns: section.
Sure, we can do convert_value. I am honestly not too fussed about
naming, and your proposal makes more sense. And as above, we can spare
the whole metadata problem. Using your example:
met
RE: [PATCH 0/4] RFC samples converting VLA to alloca
> > > > > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > > > Sent: Monday, 8 April 2024 17.27 > > > > > > > > For next technboard meeting. > > > > > > > > On Sun, Apr 07, 2024 at 10:03:06AM -0700, Stephen Hemminger wrote: > > > > > On Sun, 7 Apr 2024 13:07:06 +0200 > > > > > Morten Brørup wrote: > > > > > > > > > > > > From: Mattias Rönnblom [mailto:hof...@lysator.liu.se] > > > > > > > Sent: Sunday, 7 April 2024 11.32 > > > > > > > > > > > > > > On 2024-04-04 19:15, Tyler Retzlaff wrote: > > > > > > > > This series is not intended for merge. It insteat provides > > > > > > > > examples > > > > > > > of > > > > > > > > converting use of VLAs to alloca() would look like. > > > > > > > > > > > > > > > > what's the advantages of VLA over alloca()? > > > > > > > > > > > > > > > > * sizeof(array) works as expected. > > > > > > > > > > > > > > > > * multi-dimensional arrays are still arrays instead of pointers > > > > > > > > to > > > > > > > >dynamically allocated space. this means multiple subscript > > > > > > > > syntax > > > > > > > >works (unlike on a pointer) and calculation of addresses into > > > > > > > allocated > > > > > > > >space in ascending order is performed by the compiler > > > > > > > > instead of > > > > > > > manually. > > > > > > > > > > > > > > > > > > > > > > alloca() is a pretty obscure mechanism, and also not a part of > > > > > > > the C > > > > > > > standard. VLAs are C99, and well-known and understood, and very > > > > > > > efficient. > > > > > > > > > > > > The RFC fails to mention why we need to replace VLAs with something > > > > > > else: > > > > > > > > > > > > VLAs are C99, but not C++; VLAs were made optional in C11. > > > > > > > > > > > > MSVC doesn't support VLAs, and is not going to: > > > > > > https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support- > > > > arriving-in-msvc/#variable-length-arrays > > > > > > > > > > > > > > > > > > I dislike alloca() too, and the notes section in the alloca(3) man > > > > > > page > > > > even discourages the use of alloca(): > > > > > > https://man7.org/linux/man-pages/man3/alloca.3.html > > > > > > > > > > > > But I guess alloca() is the simplest replacement for VLAs. > > > > > > This RFC patch series opens the discussion for alternatives in > > > > > > different > > > > use cases. > > > > > > > > > > > > > > > > The other issue with VLA's is that if the number is something that > > > > > can be > > > > externally > > > > > input, then it can be a source of stack overflow bugs. That is why > > > > > the Linux > > > > kernel > > > > > has stopped using them; for security reasons. DPDK has much less of a > > > > security > > > > > trust domain. Mostly need to make sure that no data from network is > > > > > being > > > > > used to compute VLA size. > > > > > > > > > > > > > Looks like we need to discuss this at the next techboard meeting. > > > > > > > > * MSVC doesn't support C11 optional VLAs (and never will). > > > > * alloca() is an alternative that is available on all > > > > platforms/toolchain > > > > combinations. > > > > * it's reasonable for some VLAs to be turned into regular arrays but it > > > > would be unsatisfactory to be stuck waiting discussions of defining > > > > new > > > > constant expression macros on a per-use basis. > > > > > > We must generally stop using VLAs, for many reasons. > > > The only available 1:1 replacement is alloca(), so we have to accept that. > > > > > > If anyone still cares about improvements, we can turn alloca()'d arrays > > > into regular arrays after this patch series. > > > > > > Alternatives to VLAs are very interesting discussions, but let's not > > > stall MSVC progress because of it! > > > > Ok, but why we have to rush into 'alloca()' solution if none of us really > > fond of it? > > for the trivial case it is no worse than a VLA. while it isn't > standardized it is available for all platform/toolchains unlike VLA. > most of the code needed to be changed for windows falls into the trivial > case when converted. Personally, I think VLA is much more convenient then alloca(). At least you can do sizeof(vla_array) without a problem. > > there do appear to be cases where VLAs have just been unintentional. > i previously linked a patch where i fixed a case where they were > instantiated inside a cast and there are other cases i'm aware of in the > mlx5 driver where i believe they are unintended. at least with alloca > it is obvious but with a VLA if the expression used to determine the > size is wrapped up in something non-trivial and the author doesn't check > that it is truly a constant expression you get one by surprise. > > > As you already noted majority of these cases can be replaced with static > > sized arrays. > > unfortunately i don't think this is the case if we are talking about the > entire source tree. Ok, probably I misunderstood this RFC intention: My first thought that it was all you need to make
Re: [PATCH 2/6] dts: use Params for interactive shells
On Wed, Apr 10, 2024 at 11:34 AM Luca Vizzarro wrote: > > On 09/04/2024 15:56, Juraj Linkeš wrote: > > On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock wrote: > >> > >> I'm not sure if allowing None should be the solution for these shells > >> as opposed to just supplying an empty parameter object. Maybe > >> something that could be done is the factory method in sut_node allows > >> it to be None, but when it comes time to make the abstract shell it > >> creates an empty one if it doesn't exist, or the init method makes > >> this an optional parameter but creates it if it doesn't exist. > >> > >> I suppose this logic would have to exist somewhere because the > >> parameters aren't always required, it's just a question of where we > >> should put it and if we should just assume that the interactive shell > >> class just knows how to accept some parameters and put them into the > >> shell. I would maybe leave this as something that cannot be None and > >> handle it outside of the shell, but I'm not sure it's something really > >> required and I don't have a super strong opinion on it. > >> > > > > I think this is an excellent idea. An empty Params (or StrParams or > > EalParams if we want to make Params truly abstract and thus not > > instantiable) would work as the default value and it would be an > > elegant solution since dev will only pass non-empty Params. > > > > I left it as generic as it could get as I honestly couldn't grasp the > full picture. I am really keen to ditch everything else and use an empty > Params object instead for defaults. And as Juraj said, if I am making > Params a true abstract object, then it'd be picking one of the Params > subclasses mentioned above. > > I guess EalParams could only be used with shells are that sure to be > DPDK apps, and I presume that's only TestPmdShell for now. > > >>> +from framework.testbed_model.sut_node import EalParameters > >>> + > >>> +assert isinstance(self._app_args, EalParameters) > >>> + > >>> +if isinstance(self._app_args.app_params, StrParams): > >>> +self._app_args.app_params.value += " -i --mask-event > >>> intr_lsc" > >>> + > >>> +self.number_of_ports = len(self._app_args.ports) if > >>> self._app_args.ports is not None else 0 > >> > >> I we should override the _app_args parameter in the testpmd shell to > >> always be EalParameters instead of doing this import and assertion. > >> It's a DPDK app, so we will always need EalParameters anyway, so we > >> might as well put that as our typehint to start off as what we expect > >> instead. > >> > >> The checking of an instance of StrParams also feels a little strange > >> here, it might be more ideal if we could just add the parameters > >> without this check. Maybe something we can do, just because these > >> parameters are meant to be CLI commands anyway and will be rendered as > >> a string, is replace the StrParams object with a method on the base > >> Params dataclass that allows you to just add any string as a value > >> only field. Then, we don't have to bother validating anything about > >> the app parameters and we don't care what they are, we can just add a > >> string to them of new parameters. > >> > >> I think this is something that likely also gets solved when you > >> replace this with testpmd parameters, but it might be a good way to > >> make the Params object more versatile in general so that people can > >> diverge and add their own flags to it if needed. > > Jeremy, I agree this is not ideal. Although this was meant only to be > transitionary for the next commit (as you say it gets resolved with > TestPmdParams). But I agree with you that we can integrate the StrParams > facility within Params. This means no longer making Params a true > abstract class though, which is something I can live with, especially if > we can make it simpler. > No problem with it not being a true abstract class if it's not going to have abstract methods/properties. I guess making it instantiable actually makes sense, since it's always going to be empty (as there are no fields), which should make the usage mostly error-free. > > I'd say this is done because of circular imports. If so, we could move > > EalParameters to params.py, that should help. And when we're at it, > > either rename it to EalParams or rename the other classes to use the > > whole word. > > Yeah, the circular imports are the main problem indeed. I considered > refactoring but noticed it'd take more than just moving EalParam(eter)s > to params.py. Nonetheless, keen to make a bigger refactoring to make > this happen. Please do. My thinking was if we made params.py standalone we could import from it anywhere but I guess it's not that simple. :-) In any case, refactoring is always welcome - please put moved files into a separate commit. > >>> + > >>> super()._start_application(get_privileged_command) > >>> > >>> def start(self, verify: bool = True) -> None: > >> > >>>
Re: [PATCH 1/6] dts: add parameters data structure
On Wed, Apr 10, 2024 at 11:51 AM Luca Vizzarro wrote:
>
> On 10/04/2024 10:15, Juraj Linkeš wrote:
> +
> +def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +"""Injects the value of the attribute as-is without flag. Metadata
> modifier for :func:`dataclasses.field`."""
> +return {**metadata, META_VALUE_ONLY: True}
> >>>
> >>> These methods, on the other hand, are used outside this module, so it
> >>> makes sense to have them outside Params. It could be better to have
> >>> them inside as static methods, as they're closely related. Looking at
> >>> how they're used, we should unite the imports:
> >>> 1. In testpmd_shell, they're imported directly (from framework.params
> >>> import long)
> >>> 2. In sut_node, just the params module is imported (from framework
> >>> import params and then accessed via it: metadata=params.short("l"))
> >>>
> >> Having a shorter version may look less verbose. I agree that we can make
> >> everything a static method of Params, but then having to do Params.short
> >> etc everytime will make it look more verbose. So what option do we
> >> prefer? The functions do belong to the params module nonetheless, and
> >> therefore are meant to be used in conjunction with the Params class.
> >>
> >
> > When I first saw the code, I liked the usage in sut_node better, e.g.:
> > prefix: str = field(metadata=params.long("file-prefix")). I think this
> > is because it's obvious where the function comes from. I'd do the
> > longer version because I think most people are just going to glance at
> > the code when they want to know how to properly implement an argument
> > so the explicit nature could help with understanding how it should be
> > done.
>
> Ack.
>
> >>> If we move these to Params, we could import Params and use them
> >>> similarly to 2. Not sure which one is better.
> >>>
> >>
> >>
> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) ->
> dict[str, Any]:
> >>>
> >>> Any reason why mixins are plural? I've only seen this used with one
> >>> argument, do you anticipate we'd need to use more than one? We could
> >>> make this singular and if we ever need to do two things, we could just
> >>> pass a function that does those two things (or calls two different
> >>> functions). Also, I'd just rename the mixin the func or something like
> >>> that.
> >>>
> >>> The default of an argument should not be mutable, here's a quick
> >>> explanation:
> >>> https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
> >>
> >>
> >> Indeed the reason for which I create dictionaries, as I am treating them
> >> as read only. I wanted to avoid to bloat the code with lots of `is None`
> >> checks. But we can sacrifice this optimization for better code.
> >>
> >
> > This would be the only place where we'd do the check, as I don't think
> > we need the metadata argument in any of the other functions - those
> > seem to be mutually exclusive, but maybe they're not? In any case,
> > we'd need to fix this, I don't think treating them as read-only avoids
> > the problem.
> >
>
> They are not mutually exclusive. But thinking of it we can spare every
> problem with having to chain "metadata" by letting the user do it
> through the use of the pipe operator.
>
Sounds good.
> >> Here we
> >> are just chaining them for reusability. Do you have any better name in
> >> mind?
> >>
> >
> > I don't know, so let's brainstorm a bit. Let's start with the usage:
> > portmask: int | None = field(default=None, metadata=field_mixins(hex))
> >
> > Here it's not clear at all why it's called field_mixins, at least
> > compared to the other functions which are not called mixins. I guess
> > the other functions are predefined option mixins whereas we're
> > supplying our own value mixins here. I also noticed that there's a bit
> > of an inconsistency with the naming. The basic functions (long etc.)
> > don't have the "field_" prefix, but this one does. Maybe a better name
> > would be custom_mixins? Or value_mixins? Or custom_value? Or maybe
> > convert_value? I like the last one:
> > portmask: int | None = field(default=None, metadata=convert_value(hex))
> > metadata=params.convert_value(_port_to_pci,
> > metadata=params.multiple(params.short("a"))), # in sut_node
> >
> > I think this is easier to grasp. I'm thinking about whether we need to
> > have mixin(s) in the name and I don't think it adds much. If I'm a
> > developer, I'm looking at these functions and I stumble upon
> > convert_value, what I'm thinking is "Nice, I can do some conversion on
> > the values I pass, how do I do that?", then I look at the signature
> > and find out that I expect, that is I need to pass a function (or
> > multiple function if I want to). I guess this comes down to the
> > function name (field_mixins) not conveying what it's doing, rather
> > what you're passing to it.
> >
> > So my conclusion from this brainstorming is that a better name would
> > be conv
Re: [PATCH v1 1/2] dts: Improve output gathering in interactive shells
On Mon, Apr 8, 2024 at 6:20 PM Jeremy Spewock wrote: > > On Wed, Apr 3, 2024 at 5:00 AM Juraj Linkeš > wrote: > > > > On Tue, Mar 12, 2024 at 6:26 PM wrote: > > > > > > From: Jeremy Spewock > > > > > > The current implementation of consuming output from interactive shells > > > relies on being able to find an expected prompt somewhere within the > > > output buffer after sending the command. This is useful in situations > > > where the prompt does not appear in the output itself, but in some > > > practical cases (such as the starting of an XML-RPC server for scapy) > > > the prompt exists in one of the commands sent to the shell and this can > > > cause the command to exit early and creates a race condition between the > > > server starting and the first command being sent to the server. > > > > > > This patch addresses this problem by searching for a line that strictly > > > ends with the provided prompt, rather than one that simply contains it, > > > so that the detection that a command is finished is more consistent. It > > > also adds a catch to detect when a command times out before finding the > > > prompt so that the exception can be wrapped into a more explicit one and > > > display the output that it did manage to gather before timing out. > > > > > > > This could still cause problems if the prompt appears at the end of a > > line in the output. Maybe we don't need to worry about that until we > > actually hit that problem. In any case, I'd like to test this, so > > please rebase the patch. :-) > > I will rebase and send out a v2, but that is a good point. it would be > just as easy to instead check to see that the prompt is the only thing > on the line, so we could do that instead, which do you think is > better? Would this work? I'm thinking we may need to send another extra newline character to put the solitary prompt into the buffer if the command output doesn't contain a newline. > I'm sort of indifferent, I can see how verifying that the line > only contains the prompt would be useful in cases like it appears in a > docstring or something similar (and it's nice to be more explicit in > general), and I think that leaving it as the end of the line could > potentially save some verbosity if the line you are looking for is > long so you can just detect the end of it. Another problem that I > could think of with long lines potentially is if, somehow, you were > looking for a prompt that the pseudo-terminal split across a few lines > and it split your prompt, but I'm not sure this is really relevant to > us at all since we only really expect prompts that are usually short. > If it works with checking just the end of the line let's leave it like this (because of the possibility above). I think there shouldn't be any prompts without something after them in docstrings. > > > > > > +try: > > > +for line in self._stdout: > > > +out += line > > > +if line.rstrip().endswith(prompt): > > > +break > > > +except TimeoutError: > > > > I like this addition, but maybe we could do better. In the regular SSH > > session, we're expected to raise: > > * SSHSessionDeadError if the session is not alive, > > * SSHTimeoutError if the command execution times out. > > > > Can we do that here as well? > > This is a good point, I don't see why we couldn't and thinking about > it I like the idea of making this more consistent, good catch. > > > > > > +raise InteractiveCommandExecutionError( > > > > We could just reuse the SSHTimeoutError exception. Is there a reason > > to distinguish between interactive and non-interactive timeouts? > > I wanted to add a distinction between interactive and non-interactive > just because in general the way we log messages when sending commands > to interactive shells looks pretty close to what it looks like when > you do the same for non-interactive, so if there was an error I > thought it might be more helpful in the logs to know which session you > were sending the command to when an error was encountered. Maybe, > however, a better approach to this would just be keep the exception > types the same but change the messages. > There is probably some value to distinguish between them. We could just mirror the non-interactive exceptions and keep the messages the same. > > > > > > 2.43.2 > > >
RE: Strict aliasing problem with rte_eth_linkstatus_set()
> From: fengchengwen [mailto:fengcheng...@huawei.com]
> Sent: Wednesday, 10 April 2024 11.34
>
> Hi All,
>
> We have a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> we've done some
> research but haven't been able to figure out why. We'd like the community's
> help.
>
> Environment:
> 1. Source: DPDK 23.11
> 2. GCC: 12.3.1 [1]
> 3. Compiled with target kunpeng SoC (ARM64)
> 4. Run on kunpeng SoC
>
>
> Problem & Debug:
> 1. We found the hns3 driver fails to update the link status. The corresponding
> function is
> hns3_update_linkstatus_and_event [2], and we found the rte_eth_linkstatus_set
> [3] always return zero.
> 2. After disassembly the hns3_update_linkstatus_and_event, and found
> rte_eth_linkstatus_set's
> rte_atomic_exchange_explicit return to xzr register (which is zero register):
> 1239fec: 3900f3e0strbw0, [sp, #60]
> 1239ff0: 9101a041add x1, x2, #0x68 ---x2 seem not the
> variable new_link
> 1239ff4: f8ff8022swpal xzr, x2, [x1] ---this instr
> corresponding rte_atomic_exchange_explicit,
> it will place
> the resut in xzr which always zero,
> and this will
> lead to rte_eth_linkstatus_set return 0.
> 1239ff8: 3940f3e0ldrbw0, [sp, #60]
> 1239ffc: d3609c41ubfxx1, x2, #32, #8
> 123a000: 4a01eor w0, w0, w1
> 123a004: 36100080tbz w0, #2, 123a014
>
> 3. We checked other "ret = rte_eth_linkstatus_set" calls, and can't find
> similar problem.
> 4. After reading a lot of documents, we preliminarily think that the problem
> is caused by -fstrict-aliasing
> (which was enabled default with O2 or O3), if compiled with -fno-strict-
> aliasing, then this problem don't
> exist. We guest this maybe strict-aliasing's bug which only happened in our
> function.
> 5. We also try to use union to avoid such aliasing in rte_eth_linkstatus_set,
> we changed the struct
> rte_eth_link define, and it works:
> -__extension__
> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write
> */
> - uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
> - uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
> - uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
> - uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
> +struct rte_eth_link { /**< aligned for atomic64 read/write */
> + union {
> + uint64_t val64;
> + struct {
> + uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_
> */
> + uint16_t link_duplex : 1; /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
> + uint16_t link_autoneg : 1; /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
> + uint16_t link_status : 1; /**<
> RTE_ETH_LINK_[DOWN/UP] */
> + };
> + };
> };
> the corresponding rte_eth_linkstatus_set:
> @@ -1674,18 +1674,13 @@ static inline int
>rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> const struct rte_eth_link *new_link)
>{
> - RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
> >data->dev_link);
> - union {
> - uint64_t val64;
> - struct rte_eth_link link;
> - } orig;
> -
> - RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> + struct rte_eth_link old_link;
>
> - orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t
> *)new_link,
> + old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
> >dev_link.val64,
> + new_link->val64,
> rte_memory_order_seq_cst);
>
> - return (orig.link.link_status == new_link->link_status) ? -1 : 0;
> + return (old_link.link_status == new_link->link_status) ? -1 : 0;
>}
> 6. BTW: the linux kernel enabled "-fno-strict-aliasing" default, please see
> [4] for more.
>
Thank you for the detailed analysis.
Looking at the GCC documentation for -fstrict-aliasing supports your conclusion.
Just out of curiosity:
Does building with -Wstrict-aliasing=3 or -Wstrict-aliasing=1 provide any
useful information?
And are the any differences in the strict-aliasing warnings without/with your
fix?
>
> Last： We think there are two ways to solve this problem.
> 1. Add the compilation option '-fno-strict-aliasing' for hold DPDK project.
> 2. Use union to avoid such aliasing in rte_eth_linkstatus_set (please see
> above).
> PS: We prefer first way.
>
> Hope for more discuess.
>
> Thanks
Unfortunately, DPDK uses a lot of type casting where other methods would be
formally more correct.
I fear that you only found one of potentially many more bugs like th
RE: [PATCH v2 3/8] mbuf: fix Tx checksum offload examples
> > From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com] > > Sent: Tuesday, 9 April 2024 15.39 > > > > > > From: David Marchand [mailto:david.march...@redhat.com] > > > > Sent: Friday, 5 April 2024 16.46 > > > > > > > > Mandate use of rte_eth_tx_prepare() in the mbuf Tx checksum offload > > > > examples. > > > > > > I strongly disagree with this change! > > > > > > It will cause a huge performance degradation for shaping applications: > > > > > > A packet will be processed and finalized at an output or forwarding > > pipeline stage, where some other fields might also be written, so > > > zeroing e.g. the out_ip checksum at this stage has low cost (no new > > cache misses). > > > > > > Then, the packet might be queued for QoS or similar. > > > > > > If rte_eth_tx_prepare() must be called at the egress pipeline stage, > > it has to write to the packet and cause a cache miss per packet, > > > instead of simply passing on the packet to the NIC hardware. > > > > > > It must be possible to finalize the packet at the output/forwarding > > pipeline stage! > > > > If you can finalize your packet on output/forwarding, then why you > > can't invoke tx_prepare() on the same stage? > > There seems to be some misunderstanding about what tx_prepare() does - > > in fact it doesn't communicate with HW queue (doesn't update TXD ring, > > etc.), what it does - just make changes in mbuf itself. > > Yes, it reads some fields in SW TX queue struct (max number of TXDs per > > packet, etc.), but AFAIK it is safe > > to call tx_prepare() and tx_burst() from different threads. > > At least on implementations I am aware about. > > Just checked the docs - it seems not stated explicitly anywhere, might > > be that's why it causing such misunderstanding. > > > > > > > > Also, how is rte_eth_tx_prepare() supposed to work for cloned packets > > egressing on different NIC hardware? > > > > If you create a clone of full packet (including L2/L3) headers then > > obviously such construction might not > > work properly with tx_prepare() over two different NICs. > > Though In majority of cases you do clone segments with data, while at > > least L2 headers are put into different segments. > > One simple approach would be to keep L3 header in that separate segment. > > But yes, there is a problem when you'll need to send exactly the same > > packet over different NICs. > > As I remember, for bonding PMD things don't work quite well here - you > > might have a bond over 2 NICs with > > different tx_prepare() and which one to call might be not clear till > > actual PMD tx_burst() is invoked. > > > > > > > > In theory, it might get even worse if we make this opaque instead of > > transparent and standardized: > > > One PMD might reset out_ip checksum to 0x, and another PMD might > > reset it to 0x. > > > > > > > > I can only see one solution: > > > We need to standardize on common minimum requirements for how to > > prepare packets for each TX offload. > > > > If we can make each and every vendor to agree here - that definitely > > will help to simplify things quite a bit. > > An API is more than a function name and parameters. > It also has preconditions and postconditions. > > All major NIC vendors are contributing to DPDK. > It should be possible to reach consensus for reasonable minimum requirements > for offloads. > Hardware- and driver-specific exceptions can be documented with the offload > flag, or with rte_eth_rx/tx_burst(), like the note to > rte_eth_rx_burst(): > "Some drivers using vector instructions require that nb_pkts is divisible by > 4 or 8, depending on the driver implementation." If we introduce a rule that everyone supposed to follow and then straightway allow people to have a 'documented exceptions', for me it means like 'no rule' in practice. A 'documented exceptions' approach might work if you have 5 different PMDs to support, but not when you have 50+. No-one would write an app with possible 10 different exception cases in his head. Again, with such approach we can forget about backward compatibility. I think we already had this discussion before, my opinion remains the same here - 'documented exceptions' approach is a way to trouble. > You mention the bonding driver, which is a good example. > The rte_eth_tx_burst() documentation has a note about the API postcondition > exception for the bonding driver: > "This function must not modify mbufs (including packets data) unless the > refcnt is 1. An exception is the bonding PMD, [...], mbufs > may be modified." For me, what we've done for bonding tx_prepare/tx_burst() is a really bad example. Initial agreement and design choice was that tx_burst() should not modify contents of the packets (that actually was one of the reasons why tx_prepare() was introduced). The only reason I agreed on that exception - because I couldn't come-up with something less uglier. Actually, these problems with bonding PMD made me to start thinking that curr
Re: [PATCH v2] dts: Change hugepage runtime config to 2MB Exclusively
> diff --git a/dts/framework/testbed_model/linux_session.py
> b/dts/framework/testbed_model/linux_session.py
> index 5d24030c3d..37f5eacb21 100644
> --- a/dts/framework/testbed_model/linux_session.py
> +++ b/dts/framework/testbed_model/linux_session.py
> @@ -15,7 +15,7 @@
>
> from typing_extensions import NotRequired
>
> -from framework.exception import RemoteCommandExecutionError
> +from framework.exception import ConfigurationError,
> RemoteCommandExecutionError
> from framework.utils import expand_range
>
> from .cpu import LogicalCore
> @@ -87,25 +87,22 @@ def get_dpdk_file_prefix(self, dpdk_prefix: str) -> str:
> def setup_hugepages(self, hugepage_count: int, force_first_numa: bool)
> -> None:
> """Overrides :meth:`~.os_session.OSSession.setup_hugepages`."""
> self._logger.info("Getting Hugepage information.")
> -hugepage_size = self._get_hugepage_size()
> +if "hugepages-2048kB" not in self.send_command("ls
> /sys/kernel/mm/hugepages").stdout:
I have one extra point on top of Morten's suggestions (which I like).
Let's create a class variable where we store the hugepage size (2048)
and use that across the code.
> +raise ConfigurationError("2MB hugepages not supported by
> operating system")
> hugepages_total = self._get_hugepages_total()
> self._numa_nodes = self._get_numa_nodes()
>
> -if force_first_numa or hugepages_total != hugepage_count:
> +if force_first_numa or hugepages_total < hugepage_count:
> # when forcing numa, we need to clear existing hugepages
> regardless
> # of size, so they can be moved to the first numa node
> -self._configure_huge_pages(hugepage_count, hugepage_size,
> force_first_numa)
> +self._configure_huge_pages(hugepage_count, 2048,
> force_first_numa)
> else:
> self._logger.info("Hugepages already configured.")
> self._mount_huge_pages()
>
> -def _get_hugepage_size(self) -> int:
> -hugepage_size = self.send_command("awk '/Hugepagesize/ {print $2}'
> /proc/meminfo").stdout
> -return int(hugepage_size)
> -
> def _get_hugepages_total(self) -> int:
> hugepages_total = self.send_command(
> -"awk '/HugePages_Total/ { print $2 }' /proc/meminfo"
> +"cat /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages"
> ).stdout
> return int(hugepages_total)
>
> --
> 2.44.0
>
Re: [PATCH 3/6] dts: add testpmd shell params
On 09/04/2024 17:37, Juraj Linkeš wrote:
As Jeremy pointed out, going forward, this is likely to become bloated
and moving it to params.py (for example) may be better.
There's a lot of testpmd args here. I commented on the implementation
of some of them. I didn't verify that the actual values match the docs
or, god forbid, tested all of it. :-) Doing that as we start using
them is going to be good enough.
It is indeed a lot of args. I double checked most of them, so it should
be mostly correct, but unfortunately I am not 100% sure. I did notice
discrepancies between the docs and the source code of testpmd too.
Although not ideal, I am inclining to update the definitions whenever a
newly implemented test case hits a roadblock.
One thing that I don't remember if I mentioned so far, is the "XYPair".
You see --flag=X,[Y] in the docs, but I am sure to have read somewhere
this is potentially just a comma-separated multiple value.
On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro wrote:
Implement all the testpmd shell parameters into a data structure.
Signed-off-by: Luca Vizzarro
Reviewed-by: Jack Bond-Preston
Reviewed-by: Honnappa Nagarahalli
---
dts/framework/remote_session/testpmd_shell.py | 633 +-
1 file changed, 615 insertions(+), 18 deletions(-)
diff --git a/dts/framework/remote_session/testpmd_shell.py
b/dts/framework/remote_session/testpmd_shell.py
index db3abb7600..a823dc53be 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
+@str_mixins(bracketed, comma_separated)
+class TestPmdRingNUMAConfig(NamedTuple):
+"""Tuple associating DPDK port, direction of the flow and NUMA socket."""
Is there any particular order for these various classes?
No, there is no actual order, potential dependencies aside.
+
+port: int
+direction: TestPmdFlowDirection
+socket: int
+
+
+@dataclass(kw_only=True)
+class TestPmdTXOnlyForwardingMode(Params):
The three special forwarding modes should really be moved right after
TestPmdForwardingModes. Do we actually need these three in
TestPmdForwardingModes? Looks like we could just remove those from
TestPmdForwardingModes since they have to be passed separately, not as
that Enum.
Can move and no we don't really need them in TestPmdForwardingModes,
they can be hardcoded in their own special classes.
+__forward_mode: Literal[TestPmdForwardingModes.txonly] = field(
+default=TestPmdForwardingModes.txonly, init=False,
metadata=long("forward-mode")
+)
I guess this is here so that "--forward-mode=txonly" gets rendered,
right? Why the two underscored? Is that because we want to hammer home
the fact that this is init=False, a kind of internal field? I'd like
to make it like the other fields, without any underscores (or maybe
just one underscore), and documented (definitely documented).
If we remove txonly from the Enum, we could just have the string value
here. The Enums are mostly useful to give users the proper range of
values.
Correct and correct. A double underscore would ensure no access to this
field, which is fixed and only there for rendering purposes... (also the
developer doesn't get a hint from the IDE, at least not on VS code) and
in the case of TestPmdForwardingModes it would remove a potential
conflict. It can definitely be documented though.
+multi_flow: Option = field(default=None,
metadata=long("txonly-multi-flow"))
+"""Generate multiple flows."""
+segments_length: XYPair | None = field(default=None,
metadata=long("txpkts"))
+"""Set TX segment sizes or total packet length."""
+
+
+@dataclass(kw_only=True)
+class TestPmdFlowGenForwardingMode(Params):
+__forward_mode: Literal[TestPmdForwardingModes.flowgen] = field(
+default=TestPmdForwardingModes.flowgen, init=False,
metadata=long("forward-mode")
+)
+clones: int | None = field(default=None, metadata=long("flowgen-clones"))
+"""Set the number of each packet clones to be sent. Sending clones reduces
host CPU load on
+creating packets and may help in testing extreme speeds or maxing out Tx
packet performance.
+N should be not zero, but less than ‘burst’ parameter.
+"""
+flows: int | None = field(default=None, metadata=long("flowgen-flows"))
+"""Set the number of flows to be generated, where 1 <= N <= INT32_MAX."""
+segments_length: XYPair | None = field(default=None,
metadata=long("txpkts"))
+"""Set TX segment sizes or total packet length."""
+
+
+@dataclass(kw_only=True)
+class TestPmdNoisyForwardingMode(Params):
+__forward_mode: Literal[TestPmdForwardingModes.noisy] = field(
+default=TestPmdForwardingModes.noisy, init=False,
metadata=long("forward-mode")
+)
Are both of __forward_mode and forward_mode needed because we need to
render both?
Yes, this would render as `--forward-mode=noisy --noisy-forward-mode=io`
using IO as example.
+forward_mode: (
+Liter
Re: [PATCH 4/6] dts: use testpmd params for scatter test suite
On 09/04/2024 20:12, Juraj Linkeš wrote:
@@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
"""
testpmd = self.sut_node.create_interactive_shell(
TestPmdShell,
-app_parameters=StrParams(
-"--mbcache=200 "
-f"--mbuf-size={mbsize} "
-"--max-pkt-len=9000 "
-"--port-topology=paired "
-"--tx-offloads=0x8000"
+app_parameters=TestPmdParameters(
+forward_mode=TestPmdForwardingModes.mac,
+mbcache=200,
+mbuf_size=[mbsize],
+max_pkt_len=9000,
+tx_offloads=0x8000,
),
privileged=True,
)
-testpmd.set_forward_mode(TestPmdForwardingModes.mac)
Jeremy, does this change the test? Instead of configuring the fw mode
after starting testpmd, we're starting testpmd with fw mode
configured.
I am not Jeremy (please Jeremy still reply), but we discussed this on
Slack. Reading through the testpmd source code, setting arguments like
forward-mode in the command line, is the exact equivalent of calling
`set forward mode` right after start-up. So it is equivalent in theory.
If not, we should remove the testpmd.set_forward_mode method, as it's
not used anymore.
Could there be test cases that change the forward mode multiple times in
the same shell, though? As this could still be needed to cover this.
Re: [PATCH 5/6] dts: add statefulness to InteractiveShell
On 10/04/2024 07:53, Juraj Linkeš wrote: I have a general question. What are these changes for? Do you anticipate us needing this in the future? Wouldn't it be better to add it only when we need it? It's been sometime since we raised this task internally. This patch and the next one arise from some survey done on old DTS test cases. Unfortunately, I can't pinpoint. Specifically for this patch though, the timeout bit is useful in conjunction with the related change in the next. Instead of giving an optional timeout argument to all the commands where we may want to change it, aren't we better off with providing a facility to temporarily change this for the current scope? On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock wrote: On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro wrote: diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py index a2c7b30d9f..5d80061e8d 100644 --- a/dts/framework/remote_session/interactive_shell.py +++ b/dts/framework/remote_session/interactive_shell.py @@ -41,8 +41,10 @@ class InteractiveShell(ABC): _stdout: channel.ChannelFile _ssh_channel: Channel _logger: DTSLogger +__default_timeout: float Only single underscores are used for other private variables, probably better to keep that consistent with this one. I agree, I don't see a reason for the double underscore. Ack. _timeout: float _app_args: Params | None +_is_privileged: bool = False 2.34.1
Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
On 10/04/2024 08:41, Juraj Linkeš wrote: @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None if self._app_args.app_params is None: self._app_args.app_params = TestPmdParameters() -self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0 +assert isinstance(self._app_args.app_params, TestPmdParameters) + This is tricky because ideally we wouldn't have the assertion here, but I understand why it is needed because Eal parameters have app args which can be any instance of params. I'm not sure of the best way to solve this, because making testpmd parameters extend from eal would break the general scheme that you have in place, and having an extension of EalParameters that enforces this app_args is TestPmdParameters would solve the issues, but might be a little clunky. Is there a way we can use a generic to get python to just understand that, in this case, this will always be TestPmdParameters? If not I might prefer making a private class where this is TestPmdParameters, just because there aren't really any other assertions that we use elsewhere and an unexpected exception from this (even though I don't think that can happen) could cause people some issues. It might be the case that an assertion is the easiest way to deal with it though, what do you think? We could change the signature (just the type of app_args) of the init method - I think we should be able to create a type that's EalParameters with .app_params being TestPmdParameters or None. The init method would just call super(). Something like the above is basically necessary with inheritance where subclasses are all extensions (not just implementations) of the superclass (having differences in API). I believe this is indeed a tricky one. But, unfortunately, I am not understanding the solution that is being proposed. To me, it just feels like using a generic factory like: self.sut_node.create_interactive_shell(..) is one of the reasons to bring in the majority of these complexities. What do you mean by creating this new type that combines EalParams and TestPmdParams?
question about eth and vlan item in flow pattern
Hi, all, I have some questions about the sub-options for ``VLAN`` and ``ETH`` item. According to the documentation, ``has_vlan`` is sub-option of ``ETH`` item and it means that the pattern contains at least one vlan. The ``VLAN`` item is used to match tagged packets and have some sub-options such as ``vid``, ``tci``, etc. If we combine them, what should the effect be? For instance, rule-0: flow create 0 ingress pattern eth has_vlan is 1 / vlan / end actions queue index 2 / end rule-1: flow create 0 ingress pattern eth has_vlan is 1 / vlan vid is 10 / end actions queue index 2 / end For rule-0, should it match single-tagged packets only or multi-tagged only or both? That is to say, which one will take effect, `has_vlan is 1` or `vlan` or both? For rule-2, which packets should it match, with inner VLAN id 10, or outer VLAN id 10, or both 10? The hns3 driver supports only the exact matching of VLAN numer. And it is planned to adapt ``has_vlan`` and ``has_more_vlan`` to the meaning of one VLAN for hns3 driver. Therefore, if the preceding combinations are supported, we need to confirm the exact meanings. So, what are your views on the above question?
Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
On 10/04/2024 08:50, Juraj Linkeš wrote:
On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro wrote:
This commit provides a state container for TestPmdShell. It currently
only indicates whether the packet forwarding has started
or not, and the number of ports which were given to the shell.
A reminder, the commit message should explain why we're doing this
change, not what the change is.
This also fixes the behaviour of `wait_link_status_up` to use the
command timeout as inherited from InteractiveShell.
Signed-off-by: Luca Vizzarro
Reviewed-by: Jack Bond-Preston
Reviewed-by: Honnappa Nagarahalli
---
dts/framework/remote_session/testpmd_shell.py | 41 +--
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/dts/framework/remote_session/testpmd_shell.py
b/dts/framework/remote_session/testpmd_shell.py
index a823dc53be..ea1d254f86 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -678,19 +678,27 @@ def __str__(self) -> str:
return self.pci_address
+@dataclass(slots=True)
+class TestPmdState:
+"""Session state container."""
+
+#:
+packet_forwarding_started: bool = False
The same question as in the previous patch, do you anticipate this
being needed and should we add this only when it's actually used?
As answered in the previous patch. We can always drop it and do it as
needed of course.
+
+#: The number of ports which were allowed on the command-line when testpmd
was started.
+number_of_ports: int = 0
+
+
class TestPmdShell(InteractiveShell):
"""Testpmd interactive shell.
The testpmd shell users should never use
the :meth:`~.interactive_shell.InteractiveShell.send_command` method
directly, but rather
call specialized methods. If there isn't one that satisfies a need, it
should be added.
-
-Attributes:
-number_of_ports: The number of ports which were allowed on the
command-line when testpmd
-was started.
"""
-number_of_ports: int
+#: Current state
+state: TestPmdState = TestPmdState()
Assigning a value makes this a class variable, shared across all
instances. This should be initialized in __init__().
But do we actually want to do this via composition? We'd need to
access the attributes via .state all the time and I don't really like
that. We could just put them into TestPmdShell directly, initializing
them in __init__().
No problem. I separated them in fear of bloating TestPmdShell. But I
agree on the bother of adding .state
#: The path to the testpmd executable.
path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")
RE: [PATCH v2 3/8] mbuf: fix Tx checksum offload examples
> From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com] > Sent: Wednesday, 10 April 2024 12.35 > > > > From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com] > > > Sent: Tuesday, 9 April 2024 15.39 > > > > > > > > From: David Marchand [mailto:david.march...@redhat.com] > > > > > Sent: Friday, 5 April 2024 16.46 > > > > > > > > > > Mandate use of rte_eth_tx_prepare() in the mbuf Tx checksum offload > > > > > examples. > > > > > > > > I strongly disagree with this change! > > > > > > > > It will cause a huge performance degradation for shaping applications: > > > > > > > > A packet will be processed and finalized at an output or forwarding > > > pipeline stage, where some other fields might also be written, so > > > > zeroing e.g. the out_ip checksum at this stage has low cost (no new > > > cache misses). > > > > > > > > Then, the packet might be queued for QoS or similar. > > > > > > > > If rte_eth_tx_prepare() must be called at the egress pipeline stage, > > > it has to write to the packet and cause a cache miss per packet, > > > > instead of simply passing on the packet to the NIC hardware. > > > > > > > > It must be possible to finalize the packet at the output/forwarding > > > pipeline stage! > > > > > > If you can finalize your packet on output/forwarding, then why you > > > can't invoke tx_prepare() on the same stage? > > > There seems to be some misunderstanding about what tx_prepare() does - > > > in fact it doesn't communicate with HW queue (doesn't update TXD ring, > > > etc.), what it does - just make changes in mbuf itself. > > > Yes, it reads some fields in SW TX queue struct (max number of TXDs per > > > packet, etc.), but AFAIK it is safe > > > to call tx_prepare() and tx_burst() from different threads. > > > At least on implementations I am aware about. > > > Just checked the docs - it seems not stated explicitly anywhere, might > > > be that's why it causing such misunderstanding. > > > > > > > > > > > Also, how is rte_eth_tx_prepare() supposed to work for cloned packets > > > egressing on different NIC hardware? > > > > > > If you create a clone of full packet (including L2/L3) headers then > > > obviously such construction might not > > > work properly with tx_prepare() over two different NICs. > > > Though In majority of cases you do clone segments with data, while at > > > least L2 headers are put into different segments. > > > One simple approach would be to keep L3 header in that separate segment. > > > But yes, there is a problem when you'll need to send exactly the same > > > packet over different NICs. > > > As I remember, for bonding PMD things don't work quite well here - you > > > might have a bond over 2 NICs with > > > different tx_prepare() and which one to call might be not clear till > > > actual PMD tx_burst() is invoked. > > > > > > > > > > > In theory, it might get even worse if we make this opaque instead of > > > transparent and standardized: > > > > One PMD might reset out_ip checksum to 0x, and another PMD might > > > reset it to 0x. > > > > > > > > > > > I can only see one solution: > > > > We need to standardize on common minimum requirements for how to > > > prepare packets for each TX offload. > > > > > > If we can make each and every vendor to agree here - that definitely > > > will help to simplify things quite a bit. > > > > An API is more than a function name and parameters. > > It also has preconditions and postconditions. > > > > All major NIC vendors are contributing to DPDK. > > It should be possible to reach consensus for reasonable minimum requirements > for offloads. > > Hardware- and driver-specific exceptions can be documented with the offload > flag, or with rte_eth_rx/tx_burst(), like the note to > > rte_eth_rx_burst(): > > "Some drivers using vector instructions require that nb_pkts is divisible by > 4 or 8, depending on the driver implementation." > > If we introduce a rule that everyone supposed to follow and then straightway > allow people to have a 'documented exceptions', > for me it means like 'no rule' in practice. > A 'documented exceptions' approach might work if you have 5 different PMDs to > support, but not when you have 50+. > No-one would write an app with possible 10 different exception cases in his > head. > Again, with such approach we can forget about backward compatibility. > I think we already had this discussion before, my opinion remains the same > here - > 'documented exceptions' approach is a way to trouble. The "minimum requirements" should be the lowest common denominator of all NICs. Exceptions should be extremely few, for outlier NICs that still want to provide an offload and its driver is unable to live up to the minimum requirements. Any exception should require techboard approval. If a NIC/driver does not support the "minimum requirements" for an offload feature, it is not allowed to claim support for that offload feature, or needs to seek approval for an exception. As another opti
Re: question about eth and vlan item in flow pattern
Hello, 10/04/2024 13:37, Jie Hai: > Hi, all, > > I have some questions about the sub-options for ``VLAN`` and ``ETH`` item. If it is not clear in the doxygen documentation, please do not hesitate to submit a patch to make it more explicit. > According to the documentation, ``has_vlan`` is sub-option of ``ETH`` > item and it means that the pattern contains at least one vlan. Yes > The ``VLAN`` item is used to match tagged packets and have some > sub-options such as ``vid``, ``tci``, etc. > > If we combine them, what should the effect be? > For instance, > > rule-0: flow create 0 ingress pattern eth has_vlan is 1 / vlan / end > actions queue index 2 / end I don't think the item "vlan" has any impact here. I mean it is redundant with "has_vlan". > rule-1: flow create 0 ingress pattern eth has_vlan is 1 / vlan vid is > 10 / end actions queue index 2 / end > > For rule-0, should it match single-tagged packets only or multi-tagged > only or both? I would say it matches 1 VLAN or more. > That is to say, which one will take effect, `has_vlan is 1` or `vlan` > or both? I think it is redundant. The real interesting usage of "has_vlan" would be for matching untagged packets. > For rule-2, which packets should it match, with inner VLAN id 10, or You mean rule-1 > outer VLAN id 10, or both 10? In case of QinQ, I suppose specifying only 1 VLAN allows to match either inner or outer. > The hns3 driver supports only the exact matching of VLAN numer. > And it is planned to adapt ``has_vlan`` and ``has_more_vlan`` to the > meaning of one VLAN for hns3 driver. Therefore, if the preceding > combinations are supported, we need to confirm the exact meanings. > > So, what are your views on the above question? I'm adding Ori Kam, maintainer of rte_flow API to confirm.
[PATCH] net/cnxk: add RSS config via ethdev configure API
From: Sunil Kumar Kori
Currently user passed RSS configuration is ignored via
rte_eth_dev_configure() API. Instead default RSS setup is done by
driver.
Adding handling for user passed RSS configuration too via
rte_eth_dev_configure().
Signed-off-by: Sunil Kumar Kori
---
drivers/net/cnxk/cnxk_ethdev.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/cnxk/cnxk_ethdev.c b/drivers/net/cnxk/cnxk_ethdev.c
index 6b37bd877f..95a3d8aaf9 100644
--- a/drivers/net/cnxk/cnxk_ethdev.c
+++ b/drivers/net/cnxk/cnxk_ethdev.c
@@ -1384,6 +1384,13 @@ cnxk_nix_configure(struct rte_eth_dev *eth_dev)
goto free_nix_lf;
}
+ /* Overwrite default RSS setup if requested by user */
+ rc = cnxk_nix_rss_hash_update(eth_dev, &conf->rx_adv_conf.rss_conf);
+ if (rc) {
+ plt_err("Failed to configure rss rc=%d", rc);
+ goto free_nix_lf;
+ }
+
/* Init the default TM scheduler hierarchy */
rc = roc_nix_tm_init(nix);
if (rc) {
--
2.25.1
Re: [PATCH v2] MAINTAINERS: remove maintainers that bounce
On Mon, Apr 08, 2024 at 09:49:54AM -0700, Stephen Hemminger wrote: > Recent patch set for rte_memcpy, discovered that the > mail addresses for yuying.zh...@intel.com and zhouguoy...@huawei.com > could not be found. > > Signed-off-by: Stephen Hemminger > --- Acked-by: Bruce Richardson If doing a V3, the email address "lijuan...@intel.com" is also invalid and will bounce. I believe all other Intel email addresses in the MAINTAINERS file are at least valid emails that won't bounce. /Bruce
Re: [PATCH 3/6] dts: add testpmd shell params
On Wed, Apr 10, 2024 at 12:49 PM Luca Vizzarro wrote:
>
> On 09/04/2024 17:37, Juraj Linkeš wrote:
> > As Jeremy pointed out, going forward, this is likely to become bloated
> > and moving it to params.py (for example) may be better.
> >
> > There's a lot of testpmd args here. I commented on the implementation
> > of some of them. I didn't verify that the actual values match the docs
> > or, god forbid, tested all of it. :-) Doing that as we start using
> > them is going to be good enough.
>
> It is indeed a lot of args. I double checked most of them, so it should
> be mostly correct, but unfortunately I am not 100% sure. I did notice
> discrepancies between the docs and the source code of testpmd too.
> Although not ideal, I am inclining to update the definitions whenever a
> newly implemented test case hits a roadblock.
>
> One thing that I don't remember if I mentioned so far, is the "XYPair".
> You see --flag=X,[Y] in the docs, but I am sure to have read somewhere
> this is potentially just a comma-separated multiple value.
>
> > On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro wrote:
> >>
> >> Implement all the testpmd shell parameters into a data structure.
> >>
> >> Signed-off-by: Luca Vizzarro
> >> Reviewed-by: Jack Bond-Preston
> >> Reviewed-by: Honnappa Nagarahalli
> >> ---
> >> dts/framework/remote_session/testpmd_shell.py | 633 +-
> >> 1 file changed, 615 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/dts/framework/remote_session/testpmd_shell.py
> >> b/dts/framework/remote_session/testpmd_shell.py
> >> index db3abb7600..a823dc53be 100644
> >> --- a/dts/framework/remote_session/testpmd_shell.py
> >> +++ b/dts/framework/remote_session/testpmd_shell.py
> >
> >
> >
> >> +@str_mixins(bracketed, comma_separated)
> >> +class TestPmdRingNUMAConfig(NamedTuple):
> >> +"""Tuple associating DPDK port, direction of the flow and NUMA
> >> socket."""
> >
> > Is there any particular order for these various classes?
>
> No, there is no actual order, potential dependencies aside.
>
Ok, can we order them according to when they appear in the code? Maybe
they already are.
> >> +
> >> +port: int
> >> +direction: TestPmdFlowDirection
> >> +socket: int
> >> +
> >> +
> >
> >
> >
> >> +@dataclass(kw_only=True)
> >> +class TestPmdTXOnlyForwardingMode(Params):
> >
> > The three special forwarding modes should really be moved right after
> > TestPmdForwardingModes. Do we actually need these three in
> > TestPmdForwardingModes? Looks like we could just remove those from
> > TestPmdForwardingModes since they have to be passed separately, not as
> > that Enum.
>
> Can move and no we don't really need them in TestPmdForwardingModes,
> they can be hardcoded in their own special classes.
>
> >> +__forward_mode: Literal[TestPmdForwardingModes.txonly] = field(
> >> +default=TestPmdForwardingModes.txonly, init=False,
> >> metadata=long("forward-mode")
> >> +)
> >
> > I guess this is here so that "--forward-mode=txonly" gets rendered,
> > right? Why the two underscored? Is that because we want to hammer home
> > the fact that this is init=False, a kind of internal field? I'd like
> > to make it like the other fields, without any underscores (or maybe
> > just one underscore), and documented (definitely documented).
> > If we remove txonly from the Enum, we could just have the string value
> > here. The Enums are mostly useful to give users the proper range of
> > values.
> >
>
> Correct and correct. A double underscore would ensure no access to this
> field, which is fixed and only there for rendering purposes... (also the
> developer doesn't get a hint from the IDE, at least not on VS code) and
> in the case of TestPmdForwardingModes it would remove a potential
> conflict. It can definitely be documented though.
>
Ok, can we do a single underscore? I don't really see a reason for two
underscores.
> >> +multi_flow: Option = field(default=None,
> >> metadata=long("txonly-multi-flow"))
> >> +"""Generate multiple flows."""
> >> +segments_length: XYPair | None = field(default=None,
> >> metadata=long("txpkts"))
> >> +"""Set TX segment sizes or total packet length."""
> >> +
> >> +
> >> +@dataclass(kw_only=True)
> >> +class TestPmdFlowGenForwardingMode(Params):
> >> +__forward_mode: Literal[TestPmdForwardingModes.flowgen] = field(
> >> +default=TestPmdForwardingModes.flowgen, init=False,
> >> metadata=long("forward-mode")
> >> +)
> >> +clones: int | None = field(default=None,
> >> metadata=long("flowgen-clones"))
> >> +"""Set the number of each packet clones to be sent. Sending clones
> >> reduces host CPU load on
> >> +creating packets and may help in testing extreme speeds or maxing out
> >> Tx packet performance.
> >> +N should be not zero, but less than ‘burst’ parameter.
> >> +"""
> >> +flows: int | None = field(default=None,
> >> metadata=long("flowgen-flows"))
> >> +"""Set the number of flows to
Re: [PATCH 4/6] dts: use testpmd params for scatter test suite
On Wed, Apr 10, 2024 at 12:53 PM Luca Vizzarro wrote:
>
> On 09/04/2024 20:12, Juraj Linkeš wrote:
> >> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
> >> """
> >> testpmd = self.sut_node.create_interactive_shell(
> >> TestPmdShell,
> >> -app_parameters=StrParams(
> >> -"--mbcache=200 "
> >> -f"--mbuf-size={mbsize} "
> >> -"--max-pkt-len=9000 "
> >> -"--port-topology=paired "
> >> -"--tx-offloads=0x8000"
> >> +app_parameters=TestPmdParameters(
> >> +forward_mode=TestPmdForwardingModes.mac,
> >> +mbcache=200,
> >> +mbuf_size=[mbsize],
> >> +max_pkt_len=9000,
> >> +tx_offloads=0x8000,
> >> ),
> >> privileged=True,
> >> )
> >> -testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> >
> > Jeremy, does this change the test? Instead of configuring the fw mode
> > after starting testpmd, we're starting testpmd with fw mode
> > configured.
>
> I am not Jeremy (please Jeremy still reply), but we discussed this on
> Slack. Reading through the testpmd source code, setting arguments like
> forward-mode in the command line, is the exact equivalent of calling
> `set forward mode` right after start-up. So it is equivalent in theory.
>
> > If not, we should remove the testpmd.set_forward_mode method, as it's
> > not used anymore.
>
> Could there be test cases that change the forward mode multiple times in
> the same shell, though? As this could still be needed to cover this.
Yes, but we don't have such a test now. It's good practice to remove
unused code. We can still bring it back anytime, it'll be in git
history.
Re: question about eth and vlan item in flow pattern
Hi Jie, Consider the following examples: 1) flow create 0 ingress pattern eth / ipv4 proto is 17 / udp / end \ actions queue index 1 / end 2) flow create 0 ingress pattern eth / ipv4 / udp / end \ actions queue index 1 / end Generally speaking, these two rules might be equivalent, with "proto is 17" in the first one probably being redundant (the very presence of "udp" item might stand for the same match criterion -- the protocol ID being 17). And I'd be tempted to treat the "has_vlan" case similarly. Consider: 3) flow create 0 ingress pattern eth has_vlan is 1 / vlan / end \ actions queue index 1 / end 4) flow create 0 ingress pattern eth / vlan / end \ actions queue index 1 / end (rule (3) is similar to "rule-0" from your example) Rules (3) and (4) seem equivalent to me -- the "has_vlan is 1" bit might be redundant in flow (3) -- the presence of item "vlan" sort of means the same. Rule (3) and (4) should probably match both single-tagged and double-tagged packets because, in both cases, the pattern does not clarify which protocol follows the outermost VLAN tag. If one needs to exclude double-tag match, they should clarify the rule in one of the possible ways, in example: - pattern eth / vlan has_more_vlan is 0 / end - pattern eth / vlan tpid is 0x0800 / end - pattern eth / vlan / ipv4 / end (the "tpid" goes to the "hdr.eth_proto" of "struct rte_flow_item_vlan"). With regard to the question about VLAN ID match, "pattern eth / vlan vid is 10", as well as "pattern eth has_vlan is 1 / vlan vid is 10", should probably match only on the outermost tag ID, -- that is, not on the inner one and not on both. That is because the "vid is 10" criterion relates to the specific header, as per the match item it sits in. If the user wants to match on the inner VLAN ID (say, 11), they should clarify specify it as follows: - pattern eth / vlan vid is 10 / vlan vid is 11 / end - pattern eth / vlan / vlan vid is 11 / end Hopefully, the rest of DPDK vendor community can correct me in case I got that wrong. Should you have more questions, please feel free to ask such. Thank you. On Wed, 10 Apr 2024, Jie Hai wrote: Hi, all, I have some questions about the sub-options for ``VLAN`` and ``ETH`` item. According to the documentation, ``has_vlan`` is sub-option of ``ETH`` item and it means that the pattern contains at least one vlan. The ``VLAN`` item is used to match tagged packets and have some sub-options such as ``vid``, ``tci``, etc. If we combine them, what should the effect be? For instance, rule-0: flow create 0 ingress pattern eth has_vlan is 1 / vlan / end actions queue index 2 / end rule-1: flow create 0 ingress pattern eth has_vlan is 1 / vlan vid is 10 / end actions queue index 2 / end For rule-0, should it match single-tagged packets only or multi-tagged only or both? That is to say, which one will take effect, `has_vlan is 1` or `vlan` or both? For rule-2, which packets should it match, with inner VLAN id 10, or outer VLAN id 10, or both 10? The hns3 driver supports only the exact matching of VLAN numer. And it is planned to adapt ``has_vlan`` and ``has_more_vlan`` to the meaning of one VLAN for hns3 driver. Therefore, if the preceding combinations are supported, we need to confirm the exact meanings. So, what are your views on the above question?
Re: [PATCH 5/6] dts: add statefulness to InteractiveShell
On Wed, Apr 10, 2024 at 1:27 PM Luca Vizzarro wrote: > > On 10/04/2024 07:53, Juraj Linkeš wrote: > > I have a general question. What are these changes for? Do you > > anticipate us needing this in the future? Wouldn't it be better to add > > it only when we need it? > > It's been sometime since we raised this task internally. This patch and > the next one arise from some survey done on old DTS test cases. > Unfortunately, I can't pinpoint. > > Specifically for this patch though, the timeout bit is useful in > conjunction with the related change in the next. Instead of giving an > optional timeout argument to all the commands where we may want to > change it, aren't we better off with providing a facility to temporarily > change this for the current scope? > This is a good question. If the scope is just one command, then no. If it's more than one, then maybe yes. I don't know which is better. We should also consider that this would introduce a difference in API between the interactive and non-interactive sessions. Do we want to do this there as well? Also, maybe set_timeout should be a property or we could just make _timeout public. And is_privileged should just be privileged, as it's a property (which shouldn't contain a verb; if it was a method it would be a good name). > > > > On Thu, Mar 28, 2024 at 5:48 PM Jeremy Spewock wrote: > >> > >> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro > >> wrote: > >> > >>> diff --git a/dts/framework/remote_session/interactive_shell.py > >>> b/dts/framework/remote_session/interactive_shell.py > >>> index a2c7b30d9f..5d80061e8d 100644 > >>> --- a/dts/framework/remote_session/interactive_shell.py > >>> +++ b/dts/framework/remote_session/interactive_shell.py > >>> @@ -41,8 +41,10 @@ class InteractiveShell(ABC): > >>> _stdout: channel.ChannelFile > >>> _ssh_channel: Channel > >>> _logger: DTSLogger > >>> +__default_timeout: float > >> > >> Only single underscores are used for other private variables, probably > >> better to keep that consistent with this one. > >> > > > > I agree, I don't see a reason for the double underscore. > > Ack. > > > > >>> _timeout: float > >>> _app_args: Params | None > >>> +_is_privileged: bool = False > >> > >>> 2.34.1 > >>> >
Re: [PATCH v2] dts: Change hugepage runtime config to 2MB Exclusively
On Wed, Apr 10, 2024 at 3:23 AM Morten Brørup
wrote:
>
> > From: Nicholas Pratte [mailto:npra...@iol.unh.edu]
> > Sent: Tuesday, 9 April 2024 19.28
> >
> > The previous implementation configures and allocates hugepage sizes
> > based on a system default. This can lead to two problems: overallocation of
> > hugepages (which may crash the remote host), and configuration of hugepage
> > sizes that are not recommended during runtime. This new implementation
> > allows only 2MB hugepage allocation during runtime; any other unique
> > hugepage size must be configured by the end-user for initializing DTS.
> >
> > If the amount of 2MB hugepages requested exceeds the amount of 2MB
> > hugepages already configured on the system, then the system will remount
> > hugepages to cover the difference. If the amount of hugepages requested is
> > either less than or equal to the amount already configured on the system,
> > then nothing is done.
> >
> > Bugzilla ID: 1370
> > Signed-off-by: Nicholas Pratte
> > Reviewed-by: Jeremy Spewock
> > ---
> > dts/conf.yaml| 4 ++--
> > dts/framework/config/__init__.py | 4 ++--
> > dts/framework/config/conf_yaml_schema.json | 6 +++---
> > dts/framework/config/types.py| 2 +-
> > dts/framework/testbed_model/linux_session.py | 15 ++-
> > 5 files changed, 14 insertions(+), 17 deletions(-)
> >
> > diff --git a/dts/conf.yaml b/dts/conf.yaml
> > index 8068345dd5..56c3ae6f4c 100644
> > --- a/dts/conf.yaml
> > +++ b/dts/conf.yaml
> > @@ -35,7 +35,7 @@ nodes:
> > lcores: "" # use all the available logical cores
> > use_first_core: false # tells DPDK to use any physical core
> > memory_channels: 4 # tells DPDK to use 4 memory channels
> > -hugepages: # optional; if removed, will use system hugepage
> > configuration
> > +hugepages_2mb: # optional; if removed, will use system hugepage
> > configuration
> > amount: 256
> > force_first_numa: false
> > ports:
> > @@ -71,7 +71,7 @@ nodes:
> > os_driver: rdma
> > peer_node: "SUT 1"
> > peer_pci: ":00:08.1"
> > -hugepages: # optional; if removed, will use system hugepage
> > configuration
> > +hugepages_2mb: # optional; if removed, will use system hugepage
> > configuration
> > amount: 256
> > force_first_numa: false
> > traffic_generator:
> > diff --git a/dts/framework/config/__init__.py
> > b/dts/framework/config/__init__.py
> > index 4cb5c74059..b6f820e39e 100644
> > --- a/dts/framework/config/__init__.py
> > +++ b/dts/framework/config/__init__.py
> > @@ -255,8 +255,8 @@ def from_dict(
> > Either an SUT or TG configuration instance.
> > """
> > hugepage_config = None
> > -if "hugepages" in d:
> > -hugepage_config_dict = d["hugepages"]
> > +if "hugepages_2mb" in d:
> > +hugepage_config_dict = d["hugepages_2mb"]
> > if "force_first_numa" not in hugepage_config_dict:
> > hugepage_config_dict["force_first_numa"] = False
> > hugepage_config = HugepageConfiguration(**hugepage_config_dict)
> > diff --git a/dts/framework/config/conf_yaml_schema.json
> > b/dts/framework/config/conf_yaml_schema.json
> > index 4731f4511d..f4d7199523 100644
> > --- a/dts/framework/config/conf_yaml_schema.json
> > +++ b/dts/framework/config/conf_yaml_schema.json
> > @@ -146,7 +146,7 @@
> > "compiler"
> >]
> > },
> > -"hugepages": {
> > +"hugepages_2mb": {
> >"type": "object",
> >"description": "Optional hugepage configuration. If not specified,
> > hugepages won't be configured and DTS will use system configuration.",
> >"properties": {
> > @@ -253,8 +253,8 @@
> > "type": "integer",
> > "description": "How many memory channels to use. Optional,
> > defaults to 1."
> >},
> > - "hugepages": {
> > -"$ref": "#/definitions/hugepages"
> > + "hugepages_2mb": {
> > +"$ref": "#/definitions/hugepages_2mb"
> >},
> >"ports": {
> > "type": "array",
> > diff --git a/dts/framework/config/types.py b/dts/framework/config/types.py
> > index 1927910d88..016e0c3dbd 100644
> > --- a/dts/framework/config/types.py
> > +++ b/dts/framework/config/types.py
> > @@ -46,7 +46,7 @@ class NodeConfigDict(TypedDict):
> > """Allowed keys and values."""
> >
> > #:
> > -hugepages: HugepageConfigurationDict
> > +hugepages_2mb: HugepageConfigurationDict
> > #:
> > name: str
> > #:
> > diff --git a/dts/framework/testbed_model/linux_session.py
> > b/dts/framework/testbed_model/linux_session.py
> > index 5d24030c3d..37f5eacb21 100644
> > --- a/dts/framework/testbed_model/linux_session.py
> > +++ b/dts/framework/testbed_model/linux_session.py
> > @@ -15,7 +15,7 @@
> >
> > from typing_extensions import NotRequired
>
Re: [PATCH 5/6] dts: add statefulness to InteractiveShell
On 10/04/2024 14:35, Juraj Linkeš wrote: We should also consider that this would introduce a difference in API between the interactive and non-interactive sessions. Do we want to do this there as well? Could definitely add it there as well. You are referring to RemoteSession I presume, right?
[PATCH 0/1] fix GCC 13 build errors on 32-bit targets
Hi everyone, sending in a patch to resolve some build issues encountered with GCC 13.2 when building against 32-bit targets. These issues were originally reported by Luca Boccassi in one of his stable builds[1]. Although, I have noticed that this problem presents itself on more versions (e.g. 21.11) as well, and it's not specific to armv7-a as the build suggests. I haven't actually noticed the problem present itself on mainline, but it doesn't mean that it won't appear in the future. This patch should correctly fix this problem. This patch is meant to target mainline, and all the currently supported stable versions. [1] https://build.opensuse.org/package/live_build_log/home:bluca:dpdk/dpdk-22.11/Debian_Next/armv7l Luca Vizzarro (1): vhost: fix GCC 13 build error lib/vhost/virtio_net.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.34.1
[PATCH 1/1] vhost: fix GCC 13 build error
This patch resolves a build error with GCC 13 and arm/aarch32 as
targets:
In function ‘mbuf_to_desc’,
inlined from ‘vhost_enqueue_async_packed’ at
../lib/vhost/virtio_net.c:1828:6,
inlined from ‘virtio_dev_rx_async_packed’ at
../lib/vhost/virtio_net.c:1842:6,
inlined from ‘virtio_dev_rx_async_submit_packed’ at
../lib/vhost/virtio_net.c:1900:7:
../lib/vhost/virtio_net.c:1159:18: error: ‘buf_vec[0].buf_addr’ may
be used uninitialized [-Werror=maybe-uninitialized]
1159 | buf_addr = buf_vec[vec_idx].buf_addr;
| ~^~~
../lib/vhost/virtio_net.c:1160:18: error: ‘buf_vec[0].buf_iova’ may
be used uninitialized [-Werror=maybe-uninitialized]
1160 | buf_iova = buf_vec[vec_idx].buf_iova;
| ~^~~
../lib/vhost/virtio_net.c:1161:35: error: ‘buf_vec[0].buf_len’ may
be used uninitialized [-Werror=maybe-uninitialized]
1161 | buf_len = buf_vec[vec_idx].buf_len;
| ^~~~
GCC complains about the possible runtime path where the while loop
which fills buf_vec (in vhost_enqueue_async_packed) is not run. As a
consequence it correctly thinks that buf_vec is not initialized while
being accessed anyways.
This scenario is actually very unlikely as the only way this can occur
is if size has overflowed to 0. Meaning that the total packet length
would be close to UINT64_MAX (or actually UINT32_MAX). At first glance,
the code suggests that this may never happen as the type of size has
been changed to 64-bit. For a 32-bit architecture such as arm
(e.g. armv7-a) and aarch32, this still happens because the operand types
(pkt->pkt_len and sizeof) are 32-bit wide, performing 32-bit arithmetic
first (where the overflow can happen) and widening to 64-bit later.
The proposed fix simply guarantees to the compiler that the scope which
fills buf_vec is accessed at least once, while not disrupting the actual
logic. This is based on the assumption that size will always be greater
than 0, as suggested by the sizeof, and the packet length will never be
as big as UINT32_MAX, and causing an overflow.
Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
Cc: sta...@dpdk.org
Signed-off-by: Luca Vizzarro
Reviewed-by: Paul Szczepanek
Reviewed-by: Nick Connolly
---
lib/vhost/virtio_net.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 1359c5fb1f..6a2ca295f5 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1935,7 +1935,7 @@ vhost_enqueue_async_packed(struct virtio_net *dev,
else
max_tries = 1;
- while (size > 0) {
+ do {
/*
* if we tried all available ring items, and still
* can't get enough buf, it means something abnormal
@@ -1962,7 +1962,7 @@ vhost_enqueue_async_packed(struct virtio_net *dev,
avail_idx += desc_count;
if (avail_idx >= vq->size)
avail_idx -= vq->size;
- }
+ } while (size > 0);
if (unlikely(mbuf_to_desc(dev, vq, pkt, buf_vec, nr_vec, *nr_buffers,
true) < 0))
return -1;
--
2.34.1
Re: Strict aliasing problem with rte_eth_linkstatus_set()
On Wed, 10 Apr 2024 17:33:53 +0800 fengchengwen wrote: > Last： We think there are two ways to solve this problem. > 1. Add the compilation option '-fno-strict-aliasing' for hold DPDK project. > 2. Use union to avoid such aliasing in rte_eth_linkstatus_set (please see > above). > PS: We prefer first way. > Please send a patch to replace alias with union. PS: you can also override aliasing for a few lines of code with either pragma's or lots of casting. Both are messy and hard to maintain.
RE: [PATCH 01/83] examples: move alignment attribute on types
> +To: David Hunt, Distributor maintainer
> +To: Radu Nicolau and Akhil Goyal, IPsec security gateway example maintainers
>
> > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com]
> > Sent: Wednesday, 20 March 2024 16.37
> >
> > Move location of __rte_aligned(a) to new conventional location. The new
> > placement between {struct,union} and the tag allows the desired
> > alignment to be imparted on the type regardless of the toolchain being
> > used for both C and C++. Additionally, it avoids confusion by Doxygen
> > when generating documentation.
> >
> > Signed-off-by: Tyler Retzlaff
> > ---
>
> Reviewed-by: Morten Brørup
>
> A few comments for the above mentioned maintainers inline below.
>
> > diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> > index bdcada1..4cf4c9d 100644
> > --- a/examples/ipsec-secgw/ipsec.h
> > +++ b/examples/ipsec-secgw/ipsec.h
> > @@ -112,7 +112,7 @@ enum {
> > return (struct ipsec_sa *)i;
> > }
> >
> > -struct ipsec_sa {
> > +struct __rte_cache_aligned ipsec_sa {
> > struct rte_ipsec_session sessions[IPSEC_SESSION_MAX];
> > uint32_t spi;
> > struct cdev_qp *cqp[RTE_MAX_LCORE];
> > @@ -170,7 +170,7 @@ struct ipsec_sa {
> > struct rte_flow_item_esp esp_spec;
> > struct rte_flow *flow;
> > struct rte_security_session_conf sess_conf;
> > -} __rte_cache_aligned;
> > +};
> >
> > struct ipsec_xf {
> > struct rte_crypto_sym_xform a;
> > @@ -190,12 +190,12 @@ struct sa_ctx {
> > struct ipsec_sa sa[];
> > };
> >
> > -struct ipsec_mbuf_metadata {
> > +struct __rte_cache_aligned ipsec_mbuf_metadata {
> > struct ipsec_sa *sa;
> > struct rte_crypto_op cop;
> > struct rte_crypto_sym_op sym_cop;
> > uint8_t buf[32];
> > -} __rte_cache_aligned;
> > +};
> >
> > #define IS_TRANSPORT(flags) ((flags) & TRANSPORT)
> >
> > @@ -224,7 +224,7 @@ struct cdev_qp {
> > uint16_t qp;
> > uint16_t in_flight;
> > uint16_t len;
> > - struct rte_crypto_op *buf[MAX_PKT_BURST] __rte_aligned(sizeof(void
> > *));
> > + alignas(sizeof(void *)) struct rte_crypto_op *buf[MAX_PKT_BURST];
>
> Aligning a pointer to the size of a pointer is superfluous, unless the
> structure is
> packed.
>
> @Radu, @Akhil: You might want to remove these in a future patch.
Agreed, these can be removed.
>
> > };
> >
> > struct ipsec_ctx {
> > @@ -235,7 +235,7 @@ struct ipsec_ctx {
> > uint16_t nb_qps;
> > uint16_t last_qp;
> > struct cdev_qp tbl[MAX_QP_PER_LCORE];
> > - struct rte_mbuf *ol_pkts[MAX_PKT_BURST] __rte_aligned(sizeof(void
> > *));
> > + alignas(sizeof(void *)) struct rte_mbuf *ol_pkts[MAX_PKT_BURST];
> > uint16_t ol_pkts_cnt;
> > uint64_t ipv4_offloads;
> > uint64_t ipv6_offloads;
> > @@ -283,18 +283,18 @@ struct cnt_blk {
> > uint32_t cnt;
> > } __rte_packed;
> >
> > -struct lcore_rx_queue {
> > +struct __rte_cache_aligned lcore_rx_queue {
> > uint16_t port_id;
> > uint8_t queue_id;
> > void *sec_ctx;
> > -} __rte_cache_aligned;
> > +};
> >
> > struct buffer {
> > uint16_t len;
> > - struct rte_mbuf *m_table[MAX_PKT_BURST] __rte_aligned(sizeof(void
> > *));
> > + alignas(sizeof(void *)) struct rte_mbuf *m_table[MAX_PKT_BURST];
> > };
> >
> > -struct lcore_conf {
> > +struct __rte_cache_aligned lcore_conf {
> > uint16_t nb_rx_queue;
> > struct lcore_rx_queue rx_queue_list[MAX_RX_QUEUE_PER_LCORE];
> > uint16_t tx_queue_id[RTE_MAX_ETHPORTS];
RE: [EXTERNAL] [PATCH v4 13/30] security: replace use of fixed size rte_memcpy
> Automatically generated by devtools/cocci/rte_memcpy.cocci > > Signed-off-by: Stephen Hemminger > --- Acked-by: Akhil Goyal
RE: [EXTERNAL] [PATCH v4 05/30] cryptodev: replace use of fixed size rte_memcpy
> Automatically generated by devtools/cocci/rte_memcpy.cocci > > Signed-off-by: Stephen Hemminger > --- > lib/cryptodev/rte_cryptodev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Acked-by: Akhil Goyal
Re: Strict aliasing problem with rte_eth_linkstatus_set()
On 4/10/2024 11:30 AM, Morten Brørup wrote:
>> From: fengchengwen [mailto:fengcheng...@huawei.com]
>> Sent: Wednesday, 10 April 2024 11.34
>>
>> Hi All,
>>
>> We have a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
>> we've done some
>> research but haven't been able to figure out why. We'd like the community's
>> help.
>>
>> Environment:
>> 1. Source: DPDK 23.11
>> 2. GCC: 12.3.1 [1]
>> 3. Compiled with target kunpeng SoC (ARM64)
>> 4. Run on kunpeng SoC
>>
>>
>> Problem & Debug:
>> 1. We found the hns3 driver fails to update the link status. The
>> corresponding
>> function is
>> hns3_update_linkstatus_and_event [2], and we found the rte_eth_linkstatus_set
>> [3] always return zero.
>> 2. After disassembly the hns3_update_linkstatus_and_event, and found
>> rte_eth_linkstatus_set's
>> rte_atomic_exchange_explicit return to xzr register (which is zero register):
>> 1239fec: 3900f3e0strbw0, [sp, #60]
>> 1239ff0: 9101a041add x1, x2, #0x68 ---x2 seem not the
>> variable new_link
>> 1239ff4: f8ff8022swpal xzr, x2, [x1] ---this instr
>> corresponding rte_atomic_exchange_explicit,
>> it will place
>> the resut in xzr which always zero,
>> and this will
>> lead to rte_eth_linkstatus_set return 0.
>> 1239ff8: 3940f3e0ldrbw0, [sp, #60]
>> 1239ffc: d3609c41ubfxx1, x2, #32, #8
>> 123a000: 4a01eor w0, w0, w1
>> 123a004: 36100080tbz w0, #2, 123a014
>>
>> 3. We checked other "ret = rte_eth_linkstatus_set" calls, and can't find
>> similar problem.
>> 4. After reading a lot of documents, we preliminarily think that the problem
>> is caused by -fstrict-aliasing
>> (which was enabled default with O2 or O3), if compiled with -fno-strict-
>> aliasing, then this problem don't
>> exist. We guest this maybe strict-aliasing's bug which only happened in our
>> function.
>> 5. We also try to use union to avoid such aliasing in rte_eth_linkstatus_set,
>> we changed the struct
>> rte_eth_link define, and it works:
>> -__extension__
>> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
>> read/write
>> */
>> - uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
>> - uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
>> - uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
>> - uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
>> +struct rte_eth_link { /**< aligned for atomic64 read/write */
>> + union {
>> + uint64_t val64;
>> + struct {
>> + uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_
>> */
>> + uint16_t link_duplex : 1; /**<
>> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
>> + uint16_t link_autoneg : 1; /**<
>> RTE_ETH_LINK_[AUTONEG/FIXED] */
>> + uint16_t link_status : 1; /**<
>> RTE_ETH_LINK_[DOWN/UP] */
>> + };
>> + };
>> };
>> the corresponding rte_eth_linkstatus_set:
>> @@ -1674,18 +1674,13 @@ static inline int
>>rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>> const struct rte_eth_link *new_link)
>>{
>> - RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
>>> data->dev_link);
>> - union {
>> - uint64_t val64;
>> - struct rte_eth_link link;
>> - } orig;
>> -
>> - RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
>> + struct rte_eth_link old_link;
>>
>> - orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
>> uint64_t
>> *)new_link,
>> + old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
>>> dev_link.val64,
>> + new_link->val64,
>> rte_memory_order_seq_cst);
>>
>> - return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>> + return (old_link.link_status == new_link->link_status) ? -1 : 0;
>>}
>> 6. BTW: the linux kernel enabled "-fno-strict-aliasing" default, please see
>> [4] for more.
>>
>
> Thank you for the detailed analysis.
> Looking at the GCC documentation for -fstrict-aliasing supports your
> conclusion.
>
Agree that issue looks like strict aliasing violation.
> Just out of curiosity:
> Does building with -Wstrict-aliasing=3 or -Wstrict-aliasing=1 provide any
> useful information?
> And are the any differences in the strict-aliasing warnings without/with your
> fix?
>
>>
>> Last： We think there are two ways to solve this problem.
>> 1. Add the compilation option '-fno-strict-aliasing' for hold DPDK project.
>> 2. Use union to avoid such aliasing in rte_eth_linkstatus_set (please see
>> above).
>> PS
Re: Strict aliasing problem with rte_eth_linkstatus_set()
On 4/10/2024 4:27 PM, Stephen Hemminger wrote: > On Wed, 10 Apr 2024 17:33:53 +0800 > fengchengwen wrote: > >> Last： We think there are two ways to solve this problem. >> 1. Add the compilation option '-fno-strict-aliasing' for hold DPDK project. >> 2. Use union to avoid such aliasing in rte_eth_linkstatus_set (please see >> above). >> PS: We prefer first way. >> > > Please send a patch to replace alias with union. > +1 I am not sure about ABI implications, as size is not changing I expect it won't be an issue but may be good to verify with libabigail. > PS: you can also override aliasing for a few lines of code with either > pragma's > or lots of casting. Both are messy and hard to maintain.
DTS WG Meeting Minutes - April 10, 2024
# Attendees * Patrick Robb * Jeremy Spewock * Paul Szczepanek * Luca Vizzarro * Juraj Linkeš # Minutes = General Announcements * AT UNH IOL, Nick installed some Broadcom 25G nics on our DTS dev servers which the team will be able to use for testing their patches going forward. = Patch discussions * Hugepages patch: * Morten noted on the mailing list that 2mb pages may not be supported on all arches, and suggested 32 bit x86 required >4mb * Bruce noted that most distros use PAE mode to allow physical addresses >4gb, and this also means that 32 bit hugepages can be 2mb. Based on this information we are able to proceed without tracking supported hugepage sizes per arch * Juraj notes that PAE allows 32 bit systems to allow for use of more than 4GB of memory. * If we hit problems in the future with the current implementation, it can be refactored at that point to allow for more configurability. * Morten also requested that “suggested” hugepages count which conf.yaml ships with be kept at 256 * If we want to change it, we need to writeup the specific reasons why. We were originally basing it on the 1024 hugepages example given in DPDK docs system requirements, and then doubling it + buffer since users may run on 2 socket systems in which hugepages are split between numa nodes. * This justification should be in the commit message * Juraj is also going to test increasing his allocation from 256 pages and see if he can run smoke tests successfully (current failing some unit tests) * Various functions need to be renamed to clarify they are hardcoded to set 2mb huge page sizes specifically * Juraj recommends simply setting a class variable for the time being which stores HUGEPAGE_SIZE * We need to document some of the assumptions we are making, like: * If you are setting hugepages from DTS, we assume you are trying to set 2mb pages * This can be set from /doc/guides/tools/dts.rst * Improve output gathering patch: * Current process for gathering output relies on finding expected prompt within output buffer * When the output itself contains the prompt, it is a false positive * So, check for the prompt at the end of the output. * There could still be this prompt at the end of normal output, so it’s not 100% bulletproof, but watching for this should be easy to maintain * Jeremy will update this patch to make it consistent with other error messages * Testpmd statefullness / params class * Juraj provided some feedback, and Luca will submit another patch later in the month * How do we handle timeouts? Old strategy is setting a timeout per sent command. * The only place where we change the timeout is when we build dpdk * In almost all cases, we do not need to set some custom timeout for sending a command for interactive or noninteractive shells. * Assumption is that when we implement testsuites, new testpmd shell is launched every time we start a new testcase. So, changing the timeout on an instance of the testpmd shell will not affect another testcase. * If for any reason testpmd HAS to be reused for two checks, those two checks need to be within the same testcase. * Should testcases be self contained? * Paul notes that his understanding based on his experience is that testcases should be self contained, and run in any order. * From here we can set a new rule, which is that we need to open and close testpmd every time we start a new testcase * This rule needs to be documented. Just mentioning it in the testpmd class is fine. * The isolation should be enforced by the system, not a requirement placed on developers and requiring human validation * This is essentially forced anyways by having developers use the testpmd context manager * This patch also adding some features which are not used in DTS right now. Usually we don’t want to do this, but these are based on previous discussions and we want to lay the groundwork for further development in the future. The group agrees there is a high likelihood that these features will be required, thus we are comfortable bringing them in now. * https://patchwork.dpdk.org/project/dpdk/list/?series=31622 * Skip test cases based on capabilities * Jeremy has rebased his new scatter testcase off of this patch * He will run this * Jeremy is concerned that “show rxq info” is not giving correct capability information for scatter on all NICs. * We could compose a list of capabilities which we expect we will need in the near future, and Juraj could add support for those. * How to compose it? We have a rough idea of what testsuites we want to por
DTS 24.07 Roadmap
Honnappa approved this a couple weeks ago, but this was never emailed out to the DTS mailing list, so doing that now. 1) Write ethdev testsuites: Jumboframes: https://git.dpdk.org/tools/dts/tree/test_plans/jumboframes_test_plan.rst Mac Filter: https://git.dpdk.org/tools/dts/tree/test_plans/mac_filter_test_plan.rst Dynamic Queue: https://git.dpdk.org/tools/dts/tree/test_plans/dynamic_queue_test_plan.rst 2) Replace XMLRPC server with Scapy interactive shell: https://bugs.dpdk.org/show_bug.cgi?id=1374 3) Configuration schema updates: DTS Hugepages refactor: https://patchwork.dpdk.org/project/dpdk/patch/20240409172811.27866-1-npra...@iol.unh.edu/ Cut down total configuration options in schema (not all we originally added are needed): https://bugs.dpdk.org/show_bug.cgi?id=1360 Split testbed and test configuration to different files: https://bugs.dpdk.org/show_bug.cgi?id=1344 4)API Docs generation: https://patchwork.dpdk.org/project/dpdk/list/?series=30877 5)Skip test cases based on testbed capabilities: https://patches.dpdk.org/project/dpdk/patch/20240301155416.96960-1-juraj.lin...@pantheon.tech/ Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=1351 6)Rename the execution section/stage: Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=1355 7)Add support for externally compiled DPDK: Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=136
[DPDK/DTS Bug 1413] Build initial list of device capabilities required by testsuites we are trying to write this year
https://bugs.dpdk.org/show_bug.cgi?id=1413 Bug ID: 1413 Summary: Build initial list of device capabilities required by testsuites we are trying to write this year Product: DPDK Version: 24.07 Hardware: All OS: All Status: UNCONFIRMED Severity: minor Priority: Normal Component: DTS Assignee: dev@dpdk.org Reporter: pr...@iol.unh.edu CC: juraj.lin...@pantheon.tech, pr...@iol.unh.edu Target Milestone: --- https://patchwork.dpdk.org/project/dpdk/patch/20240301155416.96960-1-juraj.lin...@pantheon.tech/ Juraj has this RFC for skipping testcases based on capabilities, but it only checks for scattered_rx. He has requested a more comprehensive list of what capabilities we may need to check for going forward. I think this is a good first ticket for Prince at UNH to work on. -- You are receiving this mail because: You are the assignee for the bug.
Re: [PATCH 0/4] RFC samples converting VLA to alloca
On Wed, Apr 10, 2024 at 09:58:34AM +, Konstantin Ananyev wrote: > > > > > > > > > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > > > > Sent: Monday, 8 April 2024 17.27 > > > > > > > > > > For next technboard meeting. > > > > > > > > > > On Sun, Apr 07, 2024 at 10:03:06AM -0700, Stephen Hemminger wrote: > > > > > > On Sun, 7 Apr 2024 13:07:06 +0200 > > > > > > Morten Brørup wrote: > > > > > > > > > > > > > > From: Mattias Rönnblom [mailto:hof...@lysator.liu.se] > > > > > > > > Sent: Sunday, 7 April 2024 11.32 > > > > > > > > > > > > > > > > On 2024-04-04 19:15, Tyler Retzlaff wrote: > > > > > > > > > This series is not intended for merge. It insteat provides > > > > > > > > > examples > > > > > > > > of > > > > > > > > > converting use of VLAs to alloca() would look like. > > > > > > > > > > > > > > > > > > what's the advantages of VLA over alloca()? > > > > > > > > > > > > > > > > > > * sizeof(array) works as expected. > > > > > > > > > > > > > > > > > > * multi-dimensional arrays are still arrays instead of > > > > > > > > > pointers to > > > > > > > > >dynamically allocated space. this means multiple subscript > > > > > > > > > syntax > > > > > > > > >works (unlike on a pointer) and calculation of addresses > > > > > > > > > into > > > > > > > > allocated > > > > > > > > >space in ascending order is performed by the compiler > > > > > > > > > instead of > > > > > > > > manually. > > > > > > > > > > > > > > > > > > > > > > > > > alloca() is a pretty obscure mechanism, and also not a part of > > > > > > > > the C > > > > > > > > standard. VLAs are C99, and well-known and understood, and very > > > > > > > > efficient. > > > > > > > > > > > > > > The RFC fails to mention why we need to replace VLAs with > > > > > > > something else: > > > > > > > > > > > > > > VLAs are C99, but not C++; VLAs were made optional in C11. > > > > > > > > > > > > > > MSVC doesn't support VLAs, and is not going to: > > > > > > > https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support- > > > > > arriving-in-msvc/#variable-length-arrays > > > > > > > > > > > > > > > > > > > > > I dislike alloca() too, and the notes section in the alloca(3) > > > > > > > man page > > > > > even discourages the use of alloca(): > > > > > > > https://man7.org/linux/man-pages/man3/alloca.3.html > > > > > > > > > > > > > > But I guess alloca() is the simplest replacement for VLAs. > > > > > > > This RFC patch series opens the discussion for alternatives in > > > > > > > different > > > > > use cases. > > > > > > > > > > > > > > > > > > > The other issue with VLA's is that if the number is something that > > > > > > can be > > > > > externally > > > > > > input, then it can be a source of stack overflow bugs. That is why > > > > > > the Linux > > > > > kernel > > > > > > has stopped using them; for security reasons. DPDK has much less of > > > > > > a > > > > > security > > > > > > trust domain. Mostly need to make sure that no data from network is > > > > > > being > > > > > > used to compute VLA size. > > > > > > > > > > > > > > > > Looks like we need to discuss this at the next techboard meeting. > > > > > > > > > > * MSVC doesn't support C11 optional VLAs (and never will). > > > > > * alloca() is an alternative that is available on all > > > > > platforms/toolchain > > > > > combinations. > > > > > * it's reasonable for some VLAs to be turned into regular arrays but > > > > > it > > > > > would be unsatisfactory to be stuck waiting discussions of defining > > > > > new > > > > > constant expression macros on a per-use basis. > > > > > > > > We must generally stop using VLAs, for many reasons. > > > > The only available 1:1 replacement is alloca(), so we have to accept > > > > that. > > > > > > > > If anyone still cares about improvements, we can turn alloca()'d arrays > > > > into regular arrays after this patch series. > > > > > > > > Alternatives to VLAs are very interesting discussions, but let's not > > > > stall MSVC progress because of it! > > > > > > Ok, but why we have to rush into 'alloca()' solution if none of us really > > > fond of it? > > > > for the trivial case it is no worse than a VLA. while it isn't > > standardized it is available for all platform/toolchains unlike VLA. > > most of the code needed to be changed for windows falls into the trivial > > case when converted. > > Personally, I think VLA is much more convenient then alloca(). > At least you can do sizeof(vla_array) without a problem. > > > > > there do appear to be cases where VLAs have just been unintentional. > > i previously linked a patch where i fixed a case where they were > > instantiated inside a cast and there are other cases i'm aware of in the > > mlx5 driver where i believe they are unintended. at least with alloca > > it is obvious but with a VLA if the expression used to determine the > > size is wrapped up in something non-trivial and the author doesn't check > > that it is trul
Re: [PATCH 0/4] RFC samples converting VLA to alloca
On Wed, Apr 10, 2024 at 09:32:10AM +0200, Mattias Rönnblom wrote: > On 2024-04-08 17:53, Morten Brørup wrote: > >>From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > >>Sent: Monday, 8 April 2024 17.27 > >> > >>For next technboard meeting. > >> > >>On Sun, Apr 07, 2024 at 10:03:06AM -0700, Stephen Hemminger wrote: > >>>On Sun, 7 Apr 2024 13:07:06 +0200 > >>>Morten Brørup wrote: > >>> > >From: Mattias Rönnblom [mailto:hof...@lysator.liu.se] > >Sent: Sunday, 7 April 2024 11.32 > > > >On 2024-04-04 19:15, Tyler Retzlaff wrote: > >>This series is not intended for merge. It insteat provides examples > >of > >>converting use of VLAs to alloca() would look like. > >> > >>what's the advantages of VLA over alloca()? > >> > >>* sizeof(array) works as expected. > >> > >>* multi-dimensional arrays are still arrays instead of pointers to > >>dynamically allocated space. this means multiple subscript syntax > >>works (unlike on a pointer) and calculation of addresses into > >allocated > >>space in ascending order is performed by the compiler instead of > >manually. > >> > > > >alloca() is a pretty obscure mechanism, and also not a part of the C > >standard. VLAs are C99, and well-known and understood, and very > >efficient. > > The RFC fails to mention why we need to replace VLAs with something else: > > VLAs are C99, but not C++; VLAs were made optional in C11. > > MSVC doesn't support VLAs, and is not going to: > https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support- > >>arriving-in-msvc/#variable-length-arrays > > > I dislike alloca() too, and the notes section in the alloca(3) man page > >>even discourages the use of alloca(): > https://man7.org/linux/man-pages/man3/alloca.3.html > > But I guess alloca() is the simplest replacement for VLAs. > This RFC patch series opens the discussion for alternatives in different > >>use cases. > > >>> > >>>The other issue with VLA's is that if the number is something that can be > >>externally > >>>input, then it can be a source of stack overflow bugs. That is why the > >>>Linux > >>kernel > >>>has stopped using them; for security reasons. DPDK has much less of a > >>security > >>>trust domain. Mostly need to make sure that no data from network is being > >>>used to compute VLA size. > >>> > >> > >>Looks like we need to discuss this at the next techboard meeting. > >> > >>* MSVC doesn't support C11 optional VLAs (and never will). > >>* alloca() is an alternative that is available on all platforms/toolchain > >> combinations. > >>* it's reasonable for some VLAs to be turned into regular arrays but it > >> would be unsatisfactory to be stuck waiting discussions of defining new > >> constant expression macros on a per-use basis. > > > >We must generally stop using VLAs, for many reasons. > > What reasons would that be? And which of those reasons are not also > reasons to stop using alloca(). truncated the sentence, probably should have said where static array is not practical.
Re: [PATCH 0/4] RFC samples converting VLA to alloca
On Wed, Apr 10, 2024 at 09:27:10AM +0200, Mattias Rönnblom wrote: > On 2024-04-08 17:27, Tyler Retzlaff wrote: > >For next technboard meeting. > > > >On Sun, Apr 07, 2024 at 10:03:06AM -0700, Stephen Hemminger wrote: > >>On Sun, 7 Apr 2024 13:07:06 +0200 > >>Morten Brørup wrote: > >> > From: Mattias Rönnblom [mailto:hof...@lysator.liu.se] > Sent: Sunday, 7 April 2024 11.32 > > On 2024-04-04 19:15, Tyler Retzlaff wrote: > >This series is not intended for merge. It insteat provides examples > of > >converting use of VLAs to alloca() would look like. > > > >what's the advantages of VLA over alloca()? > > > >* sizeof(array) works as expected. > > > >* multi-dimensional arrays are still arrays instead of pointers to > >dynamically allocated space. this means multiple subscript syntax > >works (unlike on a pointer) and calculation of addresses into > allocated > >space in ascending order is performed by the compiler instead of > manually. > > alloca() is a pretty obscure mechanism, and also not a part of the C > standard. VLAs are C99, and well-known and understood, and very > efficient. > >>> > >>>The RFC fails to mention why we need to replace VLAs with something else: > >>> > >>>VLAs are C99, but not C++; VLAs were made optional in C11. > >>> > >>>MSVC doesn't support VLAs, and is not going to: > >>>https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/#variable-length-arrays > >>> > >>> > >>>I dislike alloca() too, and the notes section in the alloca(3) man page > >>>even discourages the use of alloca(): > >>>https://man7.org/linux/man-pages/man3/alloca.3.html > >>> > >>>But I guess alloca() is the simplest replacement for VLAs. > >>>This RFC patch series opens the discussion for alternatives in different > >>>use cases. > >>> > >> > >>The other issue with VLA's is that if the number is something that can be > >>externally > >>input, then it can be a source of stack overflow bugs. That is why the > >>Linux kernel > >>has stopped using them; for security reasons. DPDK has much less of a > >>security > >>trust domain. Mostly need to make sure that no data from network is being > >>used to compute VLA size. > >> > > > >Looks like we need to discuss this at the next techboard meeting. > > > >* MSVC doesn't support C11 optional VLAs (and never will). > > This is due to dogmatism, or what? Surely, a lot of Open Source > projects written for C99 will use VLAs. well the statement from the MSVC team was "VLAs provide attack vectors comparable to those of the infamous gets() — deprecated and destined to removal — for opportunities of “shifting the stack” and other exploits. For these reasons we intend not to support VLAs as an optional feature in C11" i'm only communicating that they will neve be supported not debating the reasons why. it's simply a statement in fact. > > >* alloca() is an alternative that is available on all platforms/toolchain > > combinations. > > alloca() is a poor alternative. The use of alloca() should be > restricted to situations where statically sized arrays can't do the > job. agree comletely.
RE: Strict aliasing problem with rte_eth_linkstatus_set()
> From: Stephen Hemminger [mailto:step...@networkplumber.org] > Sent: Wednesday, 10 April 2024 17.27 > > On Wed, 10 Apr 2024 17:33:53 +0800 > fengchengwen wrote: > > > Last： We think there are two ways to solve this problem. > > 1. Add the compilation option '-fno-strict-aliasing' for hold DPDK > project. > > 2. Use union to avoid such aliasing in rte_eth_linkstatus_set (please > see above). > > PS: We prefer first way. > > > > Please send a patch to replace alias with union. +1 Fixing this specific bug would be good. Instinctively, I think we should build with -fno-strict-aliasing, so the compiler doesn't make the same mistake with similar code elsewhere in DPDK. I fear there is more than this instance. I also wonder if -Wstrict-aliasing could help us instead, if we don't want -fno-strict-aliasing.
RE: [EXTERNAL] [PATCH v4 27/30] app/test: replace use of fixed size rte_memcpy
> Automatically generated by devtools/cocci/rte_memcpy.cocci > > Signed-off-by: Stephen Hemminger > --- > app/test/commands.c | 1 - > app/test/packet_burst_generator.c | 4 +-- > app/test/test_crc.c | 5 ++-- > app/test/test_cryptodev.c | 18 ++--- > app/test/test_cryptodev_asym.c | 1 - > app/test/test_cryptodev_security_pdcp.c | 1 - > app/test/test_ipsec.c | 6 ++--- > app/test/test_security_inline_proto.c | 36 - For above changes Acked-by: Akhil Goyal
Re: Strict aliasing problem with rte_eth_linkstatus_set()
On Wed, Apr 10, 2024 at 07:54:27PM +0200, Morten Brørup wrote: > > From: Stephen Hemminger [mailto:step...@networkplumber.org] > > Sent: Wednesday, 10 April 2024 17.27 > > > > On Wed, 10 Apr 2024 17:33:53 +0800 > > fengchengwen wrote: > > > > > Last： We think there are two ways to solve this problem. > > > 1. Add the compilation option '-fno-strict-aliasing' for hold DPDK > > project. > > > 2. Use union to avoid such aliasing in rte_eth_linkstatus_set (please > > see above). > > > PS: We prefer first way. > > > > > > > Please send a patch to replace alias with union. > > +1 > > Fixing this specific bug would be good. > > Instinctively, I think we should build with -fno-strict-aliasing, so the > compiler doesn't make the same mistake with similar code elsewhere in DPDK. I > fear there is more than this instance. > I also wonder if -Wstrict-aliasing could help us instead, if we don't want > -fno-strict-aliasing. agree, union is the correct way to get defined behavior. there are valuable optimizatons that the compiler can make with strict aliasing enabled so -Wstrict-aliasing is a good suggestion as opposed to disabling it. also the union won't break the abi if introduced correctly.
UNH Lab downtime
Hello, The DPDK Community Lab will be offline for a couple hours tomorrow, April 11, starting at 16:00 UTC. We are doing some Community Lab dashboard maintenance. We should be able to catch up on all patchseries submitted during that time as soon as we're back online. Thanks, Patrick
Re: Strict aliasing problem with rte_eth_linkstatus_set()
On Wed, 10 Apr 2024 19:54:27 +0200 Morten Brørup wrote: > > Please send a patch to replace alias with union. > > +1 > > Fixing this specific bug would be good. > > Instinctively, I think we should build with -fno-strict-aliasing, so the > compiler doesn't make the same mistake with similar code elsewhere in DPDK. I > fear there is more than this instance. > I also wonder if -Wstrict-aliasing could help us instead, if we don't want > -fno-strict-aliasing. Strict aliasing checks should be enabled already if you use warning_level 2 and default (debugoptimized) or release build types. Unless some part of DPDK meson config is not overriding that. https://mesonbuild.com/Builtin-options.html Warning level 2 sets -Wall and -Wextra From gcc man page -Wstrict-aliasing This option is only active when -fstrict-aliasing is active. It warns about code that might break the strict aliasing rules that the compiler is using for optimization. The warning does not catch all cases, but does attempt to catch the more common pitfalls. It is included in -Wall. It is equivalent to -Wstrict-aliasing=3 and The -fstrict-aliasing option is enabled at levels -O2, -O3, -Os.
RE: [PATCH] config/arm: add Ampere AmpereOneX platform
> -Original Message- > From: Yutang Jiang > Sent: Sunday, April 7, 2024 1:36 AM > To: dev@dpdk.org > Cc: patc...@amperecomputing.com; yutang.ji...@amperecomputing.com; > jiangyut...@os.amperecomputing.com; Ruifeng Wang > ; nd ; juraj.lin...@pantheon.tech > Subject: [PATCH] config/arm: add Ampere AmpereOneX platform > > Signed-off-by: Yutang Jiang > Signed-off-by: Yutang Jiang > --- > config/arm/arm64_ampereonex_linux_gcc | 16 > config/arm/meson.build| 19 +++ Acked-by: Wathsala Vithanage
RE: [PATCH] ARM64: Cross-Compilation Support
> From: Krishna Kanth Reddy > > Modified the Configuration file to use the latest ARM Cross-Compiler. > > Fixed the linker errors for the undefined references to the APIs > isal_deflate_init, isal_deflate, isal_inflate_init, isal_inflate, > isal_inflate_stateless, isal_deflate_stateless, isal_deflate_set_hufftables > in the > case of ARM Cross-Compilation. > > Signed-off-by: Krishna Kanth Reddy > Signed-off-by: Sebastian Brzezinka > --- > config/arm/arm64_armv8_linux_gcc | 10 +- > drivers/compress/isal/meson.build | 4 Acked-by: Wathsala Vithanage
RE: [PATCH] config/arm: add Ampere AmpereOneX platform
> > > > Signed-off-by: Yutang Jiang > > Signed-off-by: Yutang Jiang > > --- Looks like this patch is signed off by Yutang twice with two different emails. Please remove one and submit again. Thank you.
Re: Strict aliasing problem with rte_eth_linkstatus_set()
Hi Morten,
On 2024/4/10 18:30, Morten Brørup wrote:
>> From: fengchengwen [mailto:fengcheng...@huawei.com]
>> Sent: Wednesday, 10 April 2024 11.34
>>
>> Hi All,
>>
>> We have a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
>> we've done some
>> research but haven't been able to figure out why. We'd like the community's
>> help.
>>
>> Environment:
>> 1. Source: DPDK 23.11
>> 2. GCC: 12.3.1 [1]
>> 3. Compiled with target kunpeng SoC (ARM64)
>> 4. Run on kunpeng SoC
>>
>>
>> Problem & Debug:
>> 1. We found the hns3 driver fails to update the link status. The
>> corresponding
>> function is
>> hns3_update_linkstatus_and_event [2], and we found the rte_eth_linkstatus_set
>> [3] always return zero.
>> 2. After disassembly the hns3_update_linkstatus_and_event, and found
>> rte_eth_linkstatus_set's
>> rte_atomic_exchange_explicit return to xzr register (which is zero register):
>> 1239fec: 3900f3e0strbw0, [sp, #60]
>> 1239ff0: 9101a041add x1, x2, #0x68 ---x2 seem not the
>> variable new_link
>> 1239ff4: f8ff8022swpal xzr, x2, [x1] ---this instr
>> corresponding rte_atomic_exchange_explicit,
>> it will place
>> the resut in xzr which always zero,
>> and this will
>> lead to rte_eth_linkstatus_set return 0.
>> 1239ff8: 3940f3e0ldrbw0, [sp, #60]
>> 1239ffc: d3609c41ubfxx1, x2, #32, #8
>> 123a000: 4a01eor w0, w0, w1
>> 123a004: 36100080tbz w0, #2, 123a014
>>
>> 3. We checked other "ret = rte_eth_linkstatus_set" calls, and can't find
>> similar problem.
>> 4. After reading a lot of documents, we preliminarily think that the problem
>> is caused by -fstrict-aliasing
>> (which was enabled default with O2 or O3), if compiled with -fno-strict-
>> aliasing, then this problem don't
>> exist. We guest this maybe strict-aliasing's bug which only happened in our
>> function.
>> 5. We also try to use union to avoid such aliasing in rte_eth_linkstatus_set,
>> we changed the struct
>> rte_eth_link define, and it works:
>> -__extension__
>> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
>> read/write
>> */
>> - uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
>> - uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
>> - uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
>> - uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
>> +struct rte_eth_link { /**< aligned for atomic64 read/write */
>> + union {
>> + uint64_t val64;
>> + struct {
>> + uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_
>> */
>> + uint16_t link_duplex : 1; /**<
>> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
>> + uint16_t link_autoneg : 1; /**<
>> RTE_ETH_LINK_[AUTONEG/FIXED] */
>> + uint16_t link_status : 1; /**<
>> RTE_ETH_LINK_[DOWN/UP] */
>> + };
>> + };
>> };
>> the corresponding rte_eth_linkstatus_set:
>> @@ -1674,18 +1674,13 @@ static inline int
>>rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>> const struct rte_eth_link *new_link)
>>{
>> - RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
>>> data->dev_link);
>> - union {
>> - uint64_t val64;
>> - struct rte_eth_link link;
>> - } orig;
>> -
>> - RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
>> + struct rte_eth_link old_link;
>>
>> - orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
>> uint64_t
>> *)new_link,
>> + old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
>>> dev_link.val64,
>> + new_link->val64,
>> rte_memory_order_seq_cst);
>>
>> - return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>> + return (old_link.link_status == new_link->link_status) ? -1 : 0;
>>}
>> 6. BTW: the linux kernel enabled "-fno-strict-aliasing" default, please see
>> [4] for more.
>>
>
> Thank you for the detailed analysis.
> Looking at the GCC documentation for -fstrict-aliasing supports your
> conclusion.
>
> Just out of curiosity:
> Does building with -Wstrict-aliasing=3 or -Wstrict-aliasing=1 provide any
> useful information?
There are no any useful information with -Wstrict-aliasing=3
But for -Wstrict-aliasing=1, there are plenty of warnings, below is
rte_eth_linkstatus_set:
In file included from ../../dpdk/lib/ethdev/rte_ethdev.c:32:
../../dpdk/lib/ethdev/ethdev_driver.h: In function ‘rte_eth_linkstatus_set’:
../../dpdk/lib/ethdev/ethdev_driver.h:1677:67: warning: dereferencing
type-pun
[PATCH] ethdev: fix strict aliasing lead to link cannot be up
Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.
This commit use union to avoid such aliasing violation.
[1] Strict aliasing problem with rte_eth_linkstatus_set()
https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
Cc: sta...@dpdk.org
Signed-off-by: Chengwen Feng
Signed-off-by: Dengdui Huang
---
lib/ethdev/ethdev_driver.h | 23 +++
lib/ethdev/rte_ethdev.h| 16 ++--
2 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..9d831d5c84 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
rte_eth_linkstatus_set(struct rte_eth_dev *dev,
const struct rte_eth_link *new_link)
{
- RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic
*)&(dev->data->dev_link);
- union {
- uint64_t val64;
- struct rte_eth_link link;
- } orig;
-
- RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+ struct rte_eth_link old_link;
- orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t
*)new_link,
- rte_memory_order_seq_cst);
+ old_link.val64 =
rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+ new_link->val64,
+ rte_memory_order_seq_cst);
- return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+ return (old_link.link_status == new_link->link_status) ? -1 : 0;
}
/**
@@ -1701,12 +1696,8 @@ static inline void
rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
struct rte_eth_link *link)
{
- RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic
*)&(dev->data->dev_link);
- uint64_t *dst = (uint64_t *)link;
-
- RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
-
- *dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+ link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
+ rte_memory_order_seq_cst);
}
/**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..0b5d3d2318 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,16 @@ struct rte_eth_stats {
/**
* A structure used to retrieve link-level information of an Ethernet port.
*/
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
- uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
- uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
- uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
- uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+ union {
+ uint64_t val64; /**< used for atomic64 read/write */
+ struct {
+ uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
+ uint16_t link_duplex : 1; /**<
RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+ uint16_t link_autoneg : 1; /**<
RTE_ETH_LINK_[AUTONEG/FIXED] */
+ uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP]
*/
+ };
+ };
};
/**@{@name Link negotiation
--
2.17.1
Re: Strict aliasing problem with rte_eth_linkstatus_set()
Hi All, On 2024/4/11 3:58, Tyler Retzlaff wrote: > On Wed, Apr 10, 2024 at 07:54:27PM +0200, Morten Brørup wrote: >>> From: Stephen Hemminger [mailto:step...@networkplumber.org] >>> Sent: Wednesday, 10 April 2024 17.27 >>> >>> On Wed, 10 Apr 2024 17:33:53 +0800 >>> fengchengwen wrote: >>> Last： We think there are two ways to solve this problem. 1. Add the compilation option '-fno-strict-aliasing' for hold DPDK >>> project. 2. Use union to avoid such aliasing in rte_eth_linkstatus_set (please >>> see above). PS: We prefer first way. >>> >>> Please send a patch to replace alias with union. >> >> +1 >> >> Fixing this specific bug would be good. OK for this, and I already send a bugfix which use union. Thanks >> >> Instinctively, I think we should build with -fno-strict-aliasing, so the >> compiler doesn't make the same mistake with similar code elsewhere in DPDK. >> I fear there is more than this instance. >> I also wonder if -Wstrict-aliasing could help us instead, if we don't want >> -fno-strict-aliasing. > > agree, union is the correct way to get defined behavior. there are > valuable optimizatons that the compiler can make with strict aliasing > enabled so -Wstrict-aliasing is a good suggestion as opposed to > disabling it. > > also the union won't break the abi if introduced correctly. > . >
Re: [PATCH v2] build: exclude rather than include libs in MSVC build
Recheck-request: iol-intel-Functional
On Wed, Apr 3, 2024 at 2:23 PM Tyler Retzlaff
wrote:
>
> Some libraries that could be built with MSVC were not being built.
>
> Switch from explicit include to exclude of libs to get immediate CI
> coverage of libraries that already work with MSVC Windows builds.
>
> Signed-off-by: Tyler Retzlaff
> Acked-by: Stephen Hemminger
> ---
> lib/argparse/meson.build | 6 ++
> lib/dmadev/meson.build | 6 ++
> lib/fib/meson.build | 6 ++
> lib/lpm/meson.build | 6 ++
> lib/mbuf/meson.build | 6 ++
> lib/mempool/meson.build | 6 ++
> lib/meson.build | 10 --
> lib/mldev/meson.build| 6 ++
> lib/rcu/meson.build | 6 ++
> lib/stack/meson.build| 6 ++
> 10 files changed, 54 insertions(+), 10 deletions(-)
>
> diff --git a/lib/argparse/meson.build b/lib/argparse/meson.build
> index b6a08ca..8ab4c40 100644
> --- a/lib/argparse/meson.build
> +++ b/lib/argparse/meson.build
> @@ -1,6 +1,12 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2024 HiSilicon Limited.
>
> +if is_ms_compiler
> +build = false
> +reason = 'not supported building with Visual Studio Toolset'
> +subdir_done()
> +endif
> +
> sources = files('rte_argparse.c')
> headers = files('rte_argparse.h')
>
> diff --git a/lib/dmadev/meson.build b/lib/dmadev/meson.build
> index 62b0650..e66dcb6 100644
> --- a/lib/dmadev/meson.build
> +++ b/lib/dmadev/meson.build
> @@ -1,6 +1,12 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2021 HiSilicon Limited.
>
> +if is_ms_compiler
> +build = false
> +reason = 'not supported building with Visual Studio Toolset'
> +subdir_done()
> +endif
> +
> sources = files('rte_dmadev.c', 'rte_dmadev_trace_points.c')
> headers = files('rte_dmadev.h')
> indirect_headers += files('rte_dmadev_core.h', 'rte_dmadev_trace_fp.h')
> diff --git a/lib/fib/meson.build b/lib/fib/meson.build
> index ddcae06..6795f41 100644
> --- a/lib/fib/meson.build
> +++ b/lib/fib/meson.build
> @@ -2,6 +2,12 @@
> # Copyright(c) 2018 Vladimir Medvedkin
> # Copyright(c) 2019 Intel Corporation
>
> +if is_ms_compiler
> +build = false
> +reason = 'not supported building with Visual Studio Toolset'
> +subdir_done()
> +endif
> +
> sources = files('rte_fib.c', 'rte_fib6.c', 'dir24_8.c', 'trie.c')
> headers = files('rte_fib.h', 'rte_fib6.h')
> deps += ['rib']
> diff --git a/lib/lpm/meson.build b/lib/lpm/meson.build
> index 4cd4888..ae30f80 100644
> --- a/lib/lpm/meson.build
> +++ b/lib/lpm/meson.build
> @@ -1,6 +1,12 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2017 Intel Corporation
>
> +if is_ms_compiler
> +build = false
> +reason = 'not supported building with Visual Studio Toolset'
> +subdir_done()
> +endif
> +
> sources = files('rte_lpm.c', 'rte_lpm6.c')
> headers = files('rte_lpm.h', 'rte_lpm6.h')
> # since header files have different names, we can install all vector headers
> diff --git a/lib/mbuf/meson.build b/lib/mbuf/meson.build
> index 0435c5e..2cee905 100644
> --- a/lib/mbuf/meson.build
> +++ b/lib/mbuf/meson.build
> @@ -1,6 +1,12 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2017 Intel Corporation
>
> +if is_ms_compiler
> +build = false
> +reason = 'not supported building with Visual Studio Toolset'
> +subdir_done()
> +endif
> +
> sources = files(
> 'rte_mbuf.c',
> 'rte_mbuf_ptype.c',
> diff --git a/lib/mempool/meson.build b/lib/mempool/meson.build
> index 8099a56..acce66c 100644
> --- a/lib/mempool/meson.build
> +++ b/lib/mempool/meson.build
> @@ -1,6 +1,12 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2017 Intel Corporation
>
> +if is_ms_compiler
> +build = false
> +reason = 'not supported building with Visual Studio Toolset'
> +subdir_done()
> +endif
> +
> extra_flags = []
>
> foreach flag: extra_flags
> diff --git a/lib/meson.build b/lib/meson.build
> index 179a272..94d2b72 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -68,16 +68,6 @@ libraries = [
> 'node',
> ]
>
> -if is_ms_compiler
> -libraries = [
> -'log',
> -'kvargs',
> -'telemetry',
> -'eal',
> -'ring',
> -]
> -endif
> -
> always_enable = [
> 'cmdline',
> 'eal',
> diff --git a/lib/mldev/meson.build b/lib/mldev/meson.build
> index 0079ccd..2c933ba 100644
> --- a/lib/mldev/meson.build
> +++ b/lib/mldev/meson.build
> @@ -1,6 +1,12 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright (c) 2022 Marvell.
>
> +if is_ms_compiler
> +build = false
> +reason = 'not supported building with Visual Studio Toolset'
> +subdir_done()
> +endif
> +
> sources = files(
> 'rte_mldev_pmd.c',
> 'rte_mldev.c',
> diff --git a/lib/rcu/meson.build b/lib/rcu/meson.build
> index 09abc52..71143f5 100644
> --- a/lib/rcu/meson.build
> +++ b/lib/rcu/meson.build
> @@ -1,6
[PATCH] mlx5: fix race at mlx5_dev_close
From: "hepeng.0320"
mlx5_dev_close currently will set priv->sh->port[priv->dev_port -
1].nl_ih_port_id to RTE_MAX_ETHPORTS to avoid mlx5_dev_interrupt_nl_cb
to use the port's dev_private, because later the rte_eth_dev_close
will free the dev_private and set the pointer to NULL.
However, since mlx5_dev_interrupt_nl_cb is running in another thread,
I think the race still exists. So perhaps an easy fix is to wait for
1ms to avoid this race.
Signed-off-by: hepeng.0320
---
drivers/net/mlx5/mlx5.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index d1a6382..283162f 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -2457,6 +2457,10 @@ mlx5_dev_close(struct rte_eth_dev *dev)
* mlx5_os_mac_addr_flush() uses ibdev_path for retrieving
* ifindex if Netlink fails.
*/
+
+ /* Avoid race condition if mlx5_dev_interrupt_nl_cb is running. */
+ rte_delay_us_sleep(1000);
+
mlx5_free_shared_dev_ctx(priv->sh);
if (priv->domain_id != RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID) {
unsigned int c = 0;
--
2.11.0
RE: [PATCH] ethdev: fix strict aliasing lead to link cannot be up
> From: Chengwen Feng [mailto:fengcheng...@huawei.com]
> Sent: Thursday, 11 April 2024 05.08
>
> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> which will lead the hns3 NIC can't link up. The root cause is strict
> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
> [1] for more details.
>
> This commit use union to avoid such aliasing violation.
>
> [1] Strict aliasing problem with rte_eth_linkstatus_set()
> https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>
> Cc: sta...@dpdk.org
>
> Signed-off-by: Chengwen Feng
> Signed-off-by: Dengdui Huang
> ---
The patch mixes atomic and non-atomic access.
This is not new for DPDK, which used to rely on compiler built-in atomics.
I'm not sure if it needs to be changed, but my suggestion is inline below.
> lib/ethdev/ethdev_driver.h | 23 +++
> lib/ethdev/rte_ethdev.h| 16 ++--
> 2 files changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 0dbf2dd6a2..9d831d5c84 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1674,18 +1674,13 @@ static inline int
> rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> const struct rte_eth_link *new_link)
> {
> - RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
> >data->dev_link);
> - union {
> - uint64_t val64;
> - struct rte_eth_link link;
> - } orig;
> -
> - RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> + struct rte_eth_link old_link;
>
> - orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
> uint64_t *)new_link,
> - rte_memory_order_seq_cst);
> + old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
> >dev_link.val64,
old_link.val64 should be written using:
rte_atomic_store_explicit(&old_link.val64,
rte_atomic_exchange_explicit(dev_link, ...), rte_memory_order_seq_cst)
> + new_link->val64,
new_link->val64 should be read using:
rte_atomic_load_explicit(&new_link->val64, rte_memory_order_seq_cst)
> + rte_memory_order_seq_cst);
>
> - return (orig.link.link_status == new_link->link_status) ? -1 : 0;
> + return (old_link.link_status == new_link->link_status) ? -1 : 0;
> }
>
> /**
> @@ -1701,12 +1696,8 @@ static inline void
> rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> struct rte_eth_link *link)
> {
> - RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
> >dev_link);
> - uint64_t *dst = (uint64_t *)link;
> -
> - RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> -
> - *dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
> + link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
link->val64 should be written using:
rte_atomic_store_explicit(&link->val64,
rte_atomic_load_explicit(&dev->data->dev_link.val64, ...),
rte_memory_order_seq_cst)
> +rte_memory_order_seq_cst);
> }
>
> /**
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147257d6a2..0b5d3d2318 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -332,12 +332,16 @@ struct rte_eth_stats {
> /**
> * A structure used to retrieve link-level information of an Ethernet
> port.
> */
> -__extension__
> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
> read/write */
> - uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
> - uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
> */
> - uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
> - uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
> +struct rte_eth_link {
> + union {
> + uint64_t val64; /**< used for atomic64 read/write */
> + struct {
> + uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_
> */
> + uint16_t link_duplex : 1; /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
> + uint16_t link_autoneg : 1; /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
> + uint16_t link_status : 1; /**<
> RTE_ETH_LINK_[DOWN/UP] */
> + };
> + };
> };
>
> /**@{@name Link negotiation
> --
> 2.17.1
Recall: [PATCH] ethdev: fix strict aliasing lead to link cannot be up
Morten Brørup would like to recall the message, "[PATCH] ethdev: fix strict aliasing lead to link cannot be up".
RE: [PATCH] ethdev: fix strict aliasing lead to link cannot be up
> From: Chengwen Feng [mailto:fengcheng...@huawei.com]
> Sent: Thursday, 11 April 2024 05.08
>
> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> which will lead the hns3 NIC can't link up. The root cause is strict
> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
> [1] for more details.
>
> This commit use union to avoid such aliasing violation.
>
> [1] Strict aliasing problem with rte_eth_linkstatus_set()
> https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>
> Cc: sta...@dpdk.org
>
> Signed-off-by: Chengwen Feng
> Signed-off-by: Dengdui Huang
> ---
The patch mixes atomic and non-atomic access.
This is not new for DPDK, which used to rely on compiler built-in atomics.
I'm not sure it needs to be changed, but my suggestion is inline below.
I don't think it makes any practical different for 64 bit arch, but it might
for 32 bit arch.
> lib/ethdev/ethdev_driver.h | 23 +++
> lib/ethdev/rte_ethdev.h| 16 ++--
> 2 files changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 0dbf2dd6a2..9d831d5c84 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1674,18 +1674,13 @@ static inline int
> rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> const struct rte_eth_link *new_link)
> {
> - RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
> >data->dev_link);
> - union {
> - uint64_t val64;
> - struct rte_eth_link link;
> - } orig;
> -
> - RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> + struct rte_eth_link old_link;
>
> - orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
> uint64_t *)new_link,
> - rte_memory_order_seq_cst);
> + old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
> >dev_link.val64,
old_link.val64 should be written using:
rte_atomic_store_explicit(&old_link.val64, ..., rte_memory_order_seq_cst)
> + new_link->val64,
new_link->val64 should be read using:
rte_atomic_load_explicit(&new_link->val64, rte_memory_order_seq_cst)
> + rte_memory_order_seq_cst);
>
> - return (orig.link.link_status == new_link->link_status) ? -1 : 0;
> + return (old_link.link_status == new_link->link_status) ? -1 : 0;
> }
>
> /**
> @@ -1701,12 +1696,8 @@ static inline void
> rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> struct rte_eth_link *link)
> {
> - RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
> >dev_link);
> - uint64_t *dst = (uint64_t *)link;
> -
> - RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> -
> - *dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
> + link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
link->val64 should be written using:
rte_atomic_store_explicit(&link->val64, ..., rte_memory_order_seq_cst)
> +rte_memory_order_seq_cst);
> }
>
> /**
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147257d6a2..0b5d3d2318 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -332,12 +332,16 @@ struct rte_eth_stats {
> /**
> * A structure used to retrieve link-level information of an Ethernet
> port.
> */
> -__extension__
> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
> read/write */
> - uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
> - uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
> */
> - uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
> - uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
> +struct rte_eth_link {
> + union {
> + uint64_t val64; /**< used for atomic64 read/write */
The type of val64 should be:
RTE_ATOMIC(uint64_t)
> + struct {
> + uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_
> */
> + uint16_t link_duplex : 1; /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
> + uint16_t link_autoneg : 1; /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
> + uint16_t link_status : 1; /**<
> RTE_ETH_LINK_[DOWN/UP] */
> + };
> + };
> };
>
> /**@{@name Link negotiation
> --
> 2.17.1
