Flow API helpers enhancements
Subsystems (3)
cover Flow API helpers enhancements
This series brings enhancements to various rte_flow helpers:
- Allow applications to use rte_flow_error_set() by making it part of the public interface and documenting it as such.
- Address rte_flow_copy()'s limitations by replacing it with the more versatile rte_flow_conv(). This new function allows retrieving other properties such as item/action names, enabling testpmd to finally use it and get rid of duplicated code.
- Add a script (gen-rte_flow_conv-h.sh) to help with generating the resources used by rte_flow_conv(). Developers should run it when adding or modifying pattern items or actions (done as part of this series to add the missing "fuzzy" pattern item).
- Future plans for rte_flow_conv() include translating error codes to human-readable messages, so applications do not have to make their own.
All these changes address concerns raised a couple of months ago [1]. Work on these patches actually started at the time but I was unable to complete and clean them up until recently.
[1] http://dpdk.org/ml/archives/dev/2017-July/070492.html
Adrien Mazarguil (7):
ethdev: expose flow API error helper
ethdev: replace flow API object copy function
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function
ethdev: enhance flow API item/action descriptions
ethdev: generate flow API conversion header
ethdev: update flow API conversion header
MAINTAINERS | 1 + app/test-pmd/config.c | 293 ++++----------- app/test-pmd/testpmd.h | 7 +- buildtools/gen-rte_flow_conv-h.sh | 264 +++++++++++++ doc/guides/prog_guide/rte_flow.rst | 43 ++- drivers/net/failsafe/failsafe_ether.c | 6 +- drivers/net/failsafe/failsafe_flow.c | 29 +- drivers/net/failsafe/failsafe_private.h | 4 +- drivers/net/mlx4/mlx4_flow.c | 6 +- drivers/net/tap/tap_flow.c | 2 +- lib/librte_ether/Makefile | 10 + lib/librte_ether/rte_ethdev_version.map | 1 + lib/librte_ether/rte_flow.c | 539 ++++++++++++++++----------- lib/librte_ether/rte_flow.h | 275 ++++++++++++-- lib/librte_ether/rte_flow_conv.h | 345 +++++++++++++++++ lib/librte_ether/rte_flow_driver.h | 38 -- 16 files changed, 1320 insertions(+), 543 deletions(-) create mode 100755 buildtools/gen-rte_flow_conv-h.sh create mode 100644 lib/librte_ether/rte_flow_conv.h
Signature · 1 line
2.1.4
PATCH 1/7 ethdev: expose flow API error helper
rte_flow_error_set() is a convenient helper to initialize error objects.
Since there is no fundamental reason to prevent applications from using it, expose it through the public interface after modifying its return value from positive to negative. This is done for consistency with the rest of the public interface.
Documentation is updated accordingly.
--- doc/guides/prog_guide/rte_flow.rst | 23 +++++++++++++++++--- drivers/net/mlx4/mlx4_flow.c | 6 +++--- drivers/net/tap/tap_flow.c | 2 +- lib/librte_ether/rte_flow.c | 30 +++++++++++++------------- lib/librte_ether/rte_flow.h | 36 +++++++++++++++++++++++++++++++ lib/librte_ether/rte_flow_driver.h | 38 --------------------------------- 6 files changed, 75 insertions(+), 60 deletions(-)
doc/guides/prog_guide/rte_flow.rst +20 -3
@@ -1678,6 +1678,25 @@ freed by the application, however its pointer can be considered valid only as long as its associated DPDK port remains configured. Closing the underlying device or unloading the PMD invalidates it. +Helpers +------- + +Error initializer +~~~~~~~~~~~~~~~~~ + +.. code-block:: c + + static inline int + rte_flow_error_set(struct rte_flow_error *error, + int code, + enum rte_flow_error_type type, + const void *cause, + const char *message); + +This function initializes ``error`` (if non-NULL) with the provided +parameters and sets ``rte_errno`` to ``code``. A negative error ``code`` is +then returned. + Caveats -------
@@ -1743,13 +1762,11 @@ the legacy filtering framework, which should eventually disappear. whatsoever). They only make sure these callbacks are non-NULL or return the ``ENOSYS`` (function not supported) error. -This interface additionally defines the following helper functions: +This interface additionally defines the following helper function: - ``rte_flow_ops_get()``: get generic flow operations structure from a port. -- ``rte_flow_error_set()``: initialize generic flow error structure. - More will be added over time. Device compatibility
drivers/net/mlx4/mlx4_flow.c +3 -3
@@ -955,9 +955,9 @@ mlx4_flow_isolate(struct rte_eth_dev *dev,mlx4_mac_addr_del (priv); }else if (mlx4_mac_addr_add (priv) <0 ) { priv->isolated =1 ; -return -rte_flow_error_set (error, rte_errno, -RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,"cannot leave isolated mode" ); +return rte_flow_error_set (error, rte_errno, +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,"cannot leave isolated mode" ); }return 0 ; }
drivers/net/tap/tap_flow.c +1 -1
@@ -1447,7 +1447,7 @@ tap_flow_isolate(struct rte_eth_dev *dev,return 0 ; error: pmd->flow_isolate =0 ; -return -rte_flow_error_set ( +return rte_flow_error_set ( error,ENOTSUP ,RTE_FLOW_ERROR_TYPE_UNSPECIFIED ,NULL ,"TC rule creation failed" ); }
lib/librte_ether/rte_flow.c +15 -15
@@ -145,9 +145,9 @@ rte_flow_validate(uint8_t port_id,return -rte_errno;if (likely (!!ops->validate))return ops->validate (dev, attr, pattern, actions, error); -return -rte_flow_error_set (error,ENOSYS , -RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,rte_strerror (ENOSYS )); +return rte_flow_error_set (error,ENOSYS , +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,rte_strerror (ENOSYS )); }/* Create a flow rule on a given port. */
@@ -183,9 +183,9 @@ rte_flow_destroy(uint8_t port_id,return -rte_errno;if (likely (!!ops->destroy))return ops->destroy (dev, flow, error); -return -rte_flow_error_set (error,ENOSYS , -RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,rte_strerror (ENOSYS )); +return rte_flow_error_set (error,ENOSYS , +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,rte_strerror (ENOSYS )); }/* Destroy all flow rules associated with a port. */
@@ -200,9 +200,9 @@ rte_flow_flush(uint8_t port_id,return -rte_errno;if (likely (!!ops->flush))return ops->flush (dev, error); -return -rte_flow_error_set (error,ENOSYS , -RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,rte_strerror (ENOSYS )); +return rte_flow_error_set (error,ENOSYS , +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,rte_strerror (ENOSYS )); }/* Query an existing flow rule. */
@@ -220,9 +220,9 @@ rte_flow_query(uint8_t port_id,return -rte_errno;if (likely (!!ops->query))return ops->query (dev, flow, action, data, error); -return -rte_flow_error_set (error,ENOSYS , -RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,rte_strerror (ENOSYS )); +return rte_flow_error_set (error,ENOSYS , +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,rte_strerror (ENOSYS )); }/* Restrict ingress traffic to the defined flow rules. */
@@ -238,9 +238,9 @@ rte_flow_isolate(uint8_t port_id,return -rte_errno;if (likely (!!ops->isolate))return ops->isolate (dev, set, error); -return -rte_flow_error_set (error,ENOSYS , -RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,rte_strerror (ENOSYS )); +return rte_flow_error_set (error,ENOSYS , +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,rte_strerror (ENOSYS )); }/** Compute storage space needed by item specification. */
lib/librte_ether/rte_flow.h +36 -0
@@ -43,6 +43,7 @@*/ #include <rte_arp.h> +#include <rte_errno.h> #include <rte_ether.h> #include <rte_icmp.h> #include <rte_ip.h>
@@ -1270,6 +1271,41 @@ int rte_flow_isolate(uint8_t port_id, int set, struct rte_flow_error *error);/** +* Initialize flow error structure. +* +* @param[out] error +* Pointer to flow error structure (may be NULL). +* @param code +* Related error code (rte_errno). +* @param type +* Cause field and error types. +* @param cause +* Object responsible for the error. +* @param message +* Human-readable error message. +* +* @return +* Negative error code (errno value) and rte_errno is set. +*/ +static inline int +rte_flow_error_set (struct rte_flow_error *error, +int code, +enum rte_flow_error_type type, +const void *cause, +const char *message) +{ +if (error) { + *error = (struct rte_flow_error ){ + .type = type, + .cause = cause, + .message = message, + }; + } + rte_errno = code; +return -code; +} + +/** * Generic flow representation. * * This form is sufficient to describe an rte_flow independently from any
lib/librte_ether/rte_flow_driver.h +0 -38
@@ -45,7 +45,6 @@#include <stdint.h> -#include <rte_errno.h> #include "rte_ethdev.h" #include "rte_flow.h"
@@ -128,43 +127,6 @@ struct rte_flow_ops { };/** -* Initialize generic flow error structure. -* -* This function also sets rte_errno to a given value. -* -* @param[out] error -* Pointer to flow error structure (may be NULL). -* @param code -* Related error code (rte_errno). -* @param type -* Cause field and error types. -* @param cause -* Object responsible for the error. -* @param message -* Human-readable error message. -* -* @return -* Error code. -*/ -static inline int -rte_flow_error_set (struct rte_flow_error *error, -int code, -enum rte_flow_error_type type, -const void *cause, -const char *message) -{ -if (error) { - *error = (struct rte_flow_error ){ - .type = type, - .cause = cause, - .message = message, - }; - } - rte_errno = code; -return code; -} - -/** * Get generic flow operations structure from a port. * * @param port_id
Signature · 1 line
2.1.4
Re: [PATCH v1 1/7] ethdev: expose flow API error helper
05/10/2017 11:49, Adrien Mazarguil:
Quoted 26 lines
rte_flow_error_set()is a convenient helper to initialize error objects.Since there is no fundamental reason to prevent applications from using it, expose it through the public interface after modifying its return value from positive to negative. This is done for consistency with the rest of the public interface.
Documentation is updated accordingly.
Signed-off-by Adrien Mazarguil <[email protected]>---
lib/librte_ether/rte_flow.h +12
-0/** +* Initialize flow error structure. +* +* @param[out] error +* Pointer to flow error structure (may be NULL). +* @param code +* Related error code (rte_errno). +* @param type +* Cause field and error types. +* @param cause +* Object responsible for the error. +* @param message +* Human-readable error message.
lib/librte_ether/rte_flow.h int · line 1295
Quoted 10 lines
lib/librte_ether/rte_flow.h +10
-0+* +* @return +* Negative error code (errno value) and rte_errno is set. +*/ +static inline int +rte_flow_error_set (struct rte_flow_error *error, +int code, +enum rte_flow_error_type type, +const void *cause, +const char *message)
When calling this function, the performance is not critical. So it must not be an inline function.
Please move it to the .c file and add it to the .map file.
Re: [PATCH v1 1/7] ethdev: expose flow API error helper
Quoted 33 lines · Thomas Monjalon
05/10/2017 11:49, Adrien Mazarguil:
Quoted 32 lines
rte_flow_error_set()is a convenient helper to initialize error objects.Since there is no fundamental reason to prevent applications from using it, expose it through the public interface after modifying its return value from positive to negative. This is done for consistency with the rest of the public interface.
Documentation is updated accordingly.
Signed-off-by Adrien Mazarguil <[email protected]>---
lib/librte_ether/rte_flow.h +18
-0/** +* Initialize flow error structure. +* +* @param[out] error +* Pointer to flow error structure (may be NULL). +* @param code +* Related error code (rte_errno). +* @param type +* Cause field and error types. +* @param cause +* Object responsible for the error. +* @param message +* Human-readable error message. +* +* @return +* Negative error code (errno value) and rte_errno is set. +*/ +static inline int +rte_flow_error_set(struct rte_flow_error *error,
Quoted 8 lines
lib/librte_ether/rte_flow.h int · line 1295
Quoted 4 lines
lib/librte_ether/rte_flow.h +4
-0+int code, + enum rte_flow_error_type type, + const void *cause, + const char *message)When calling this function, the performance is not critical. So it must not be an inline function.
Please move it to the .c file and add it to the .map file.
I'll do it as part of moving this patch into my upcoming series for mlx4, since it relies on the new return value.
Now for the record, I believe that static inlines, particularly short ones that are not expected to be modified any time soon (provided they are defined properly that is) are harmless.
Yes, that means modifying them, depending on what they do, may cause a global ABI breakage of the underlying library which translates to a major version bump, however exactly the same applies to some macros (in particular the *_MAX ones), structure and other type definitions and so on.
What I'm getting at is, making this function part of the .map file won't help all that much with the goal of reaching a stable ABI. Making it a normal function on the other hand successfully prevents PMDs and applications from considering its use in their data plane (flow rules management from the data plane is likely the next step for rte_flow).
In my opinion, performance (even if hypothetical) matters more than ABI stability, particularly since the current pattern is for the whole ABI to be broken every other release. I think that with DPDK, ABI stability can only be properly guaranteed in stable/maintenance releases but that effort is vain in the master branch, affected by way too many changes for it to ever become reality. This doesn't mean we shouldn't even try, however we shouldn't restrict ourselves; when the ABI needs to be broken, so be it.
(end of rant, thanks for reading)
Signature · 2 lines
Adrien Mazarguil 6WIND
Re: [PATCH v1 1/7] ethdev: expose flow API error helper
Quoted 46 lines · Adrien Mazarguil
Quoted 33 lines · Thomas Monjalon
05/10/2017 11:49, Adrien Mazarguil:
Quoted 32 lines
rte_flow_error_set()is a convenient helper to initialize error objects.Since there is no fundamental reason to prevent applications from using it, expose it through the public interface after modifying its return value from positive to negative. This is done for consistency with the rest of the public interface.
Documentation is updated accordingly.
Signed-off-by Adrien Mazarguil <[email protected]>---
lib/librte_ether/rte_flow.h +18
-0/** +* Initialize flow error structure. +* +* @param[out] error +* Pointer to flow error structure (may be NULL). +* @param code +* Related error code (rte_errno). +* @param type +* Cause field and error types. +* @param cause +* Object responsible for the error. +* @param message +* Human-readable error message. +* +* @return +* Negative error code (errno value) and rte_errno is set. +*/ +static inline int +rte_flow_error_set(struct rte_flow_error *error,Quoted 8 lines
lib/librte_ether/rte_flow.h int · line 1295
Quoted 4 lines
lib/librte_ether/rte_flow.h +4
-0+int code, + enum rte_flow_error_type type, + const void *cause, + const char *message)When calling this function, the performance is not critical. So it must not be an inline function.
Please move it to the .c file and add it to the .map file.I'll do it as part of moving this patch into my upcoming series for mlx4, since it relies on the new return value.
Quoted 8 lines
Now for the record, I believe that static inlines, particularly short ones that are not expected to be modified any time soon (provided they are defined properly that is) are harmless.
Yes, that means modifying them, depending on what they do, may cause a global ABI breakage of the underlying library which translates to a major version bump, however exactly the same applies to some macros (in particular the
*_MAXones), structure and other type definitions and so on.
Sure. I think if it's small and there's never a chance for it to change, then it's okay. But, as you point out - they are similar to macros in that they are code directly added to the application. There is never a way to fix a bug or adjust some semantic behavior without requiring a recompile. That's a strong kind of binding. Certainly we would want that communicated as ABI, regardless of which .h/.c file the function exists, yes?
Quoted 5 lines
What I'm getting at is, making this function part of the .map file won't help all that much with the goal of reaching a stable ABI. Making it a normal function on the other hand successfully prevents PMDs and applications from considering its use in their data plane (flow rules management from the data plane is likely the next step for
rte_flow).
I don't understand this point well. There are two things - committing to a stable ABI, and communicating information about that ABI. Having .map files doesn't do anything for a stable ABI. It merely communicates information. The stability must come from the developer community and maintainers.
Quoted 7 lines
In my opinion, performance (even if hypothetical) matters more than ABI stability, particularly since the current pattern is for the whole ABI to be broken every other release. I think that with DPDK, ABI stability can only be properly guaranteed in stable/maintenance releases but that effort is vain in the master branch, affected by way too many changes for it to ever become reality. This doesn't mean we shouldn't even try, however we shouldn't restrict ourselves; when the ABI needs to be broken, so be it.
First, performance isn't hypothetical. You have code which performs, and have measured various metrics to show this, or you don't have data. No voodoo required. Hypothetical isn't even an argument when it comes to predictable devices (like computers). In the case above, you have a non-performance critical function. It's an error function. It passes strings around. Performance cannot be a consideration here.
Second, it depends on where you want to view the boundary. If a change is internal to DPDK (meaning, the application is never expected to interact with that area of code), then it shouldn't be part of ABI and changing it to your hearts content should be fine.
*rant-on*
I see lots of projects which use DPDK, but I only know of three or so projects which don't directly embed a copy of the DPDK source tree. I think this is a side effect of not having a stable ABI - it's not possible to rely on easily upgrading the library, so people just pick a version and never upgrade. That's bad for everyone. It results in fewer contributions, and difficulty getting people to participate. There's too much text like "Install DPDK 1.8.0. With other versions is not guaranted it works properly" that is embedded in downstream users (that comes from an actual project README). Even at DPDK summit in Dublin, one of the most vocal opponents to ABI stability admitted that his company takes the code and never contributes it back. That is really not friendly.
This even has a different side effect - one of market penetration. It's difficult for distributions to support this model. So, there will be one or two packages they might pick up, with one or two versions. Everything else is "roll your own." And when it's more difficult than 'apt-get|yum|dnf|foo install' then users don't use it, and the cycle continues.
I'll point out that I can take a number of compiled binaries from 20 years ago[0], and run on a modern kernel. I wish I could take a binary linked to DPDK from 5 months ago, and run it with today's DPDK. I believe that *should* be the goal. I am confident it's achievable. But, I recognize it's hard and requires extra effort.
Anyway, apologies if I took some bait here, and sorry that I ranted off on my own.
Quoted 1 line
(end of rant, thanks for reading)
[0]: https://www.complang.tuwien.ac.at/anton/linux-binary-compatibility.html
Re: [PATCH v1 1/7] ethdev: expose flow API error helper
Quoted 119 lines · Aaron Conole
Quoted 46 lines · Adrien Mazarguil
Quoted 33 lines · Thomas Monjalon
05/10/2017 11:49, Adrien Mazarguil:
Quoted 32 lines
rte_flow_error_set()is a convenient helper to initialize error objects.Since there is no fundamental reason to prevent applications from using it, expose it through the public interface after modifying its return value from positive to negative. This is done for consistency with the rest of the public interface.
Documentation is updated accordingly.
Signed-off-by Adrien Mazarguil <[email protected]>---
lib/librte_ether/rte_flow.h +18
-0/** +* Initialize flow error structure. +* +* @param[out] error +* Pointer to flow error structure (may be NULL). +* @param code +* Related error code (rte_errno). +* @param type +* Cause field and error types. +* @param cause +* Object responsible for the error. +* @param message +* Human-readable error message. +* +* @return +* Negative error code (errno value) and rte_errno is set. +*/ +static inline int +rte_flow_error_set(struct rte_flow_error *error,Quoted 8 lines
lib/librte_ether/rte_flow.h int · line 1295
Quoted 4 lines
lib/librte_ether/rte_flow.h +4
-0+int code, + enum rte_flow_error_type type, + const void *cause, + const char *message)When calling this function, the performance is not critical. So it must not be an inline function.
Please move it to the .c file and add it to the .map file.I'll do it as part of moving this patch into my upcoming series for mlx4, since it relies on the new return value.
Quoted 8 lines
Now for the record, I believe that static inlines, particularly short ones that are not expected to be modified any time soon (provided they are defined properly that is) are harmless.
Yes, that means modifying them, depending on what they do, may cause a global ABI breakage of the underlying library which translates to a major version bump, however exactly the same applies to some macros (in particular the
*_MAXones), structure and other type definitions and so on.Sure. I think if it's small and there's never a chance for it to change, then it's okay. But, as you point out - they are similar to macros in that they are code directly added to the application. There is never a way to fix a bug or adjust some semantic behavior without requiring a recompile. That's a strong kind of binding. Certainly we would want that communicated as ABI, regardless of which .h/.c file the function exists, yes?
Quoted 5 lines
What I'm getting at is, making this function part of the .map file won't help all that much with the goal of reaching a stable ABI. Making it a normal function on the other hand successfully prevents PMDs and applications from considering its use in their data plane (flow rules management from the data plane is likely the next step for
rte_flow).I don't understand this point well. There are two things - committing to a stable ABI, and communicating information about that ABI. Having .map files doesn't do anything for a stable ABI. It merely communicates information. The stability must come from the developer community and maintainers.
Quoted 7 lines
In my opinion, performance (even if hypothetical) matters more than ABI stability, particularly since the current pattern is for the whole ABI to be broken every other release. I think that with DPDK, ABI stability can only be properly guaranteed in stable/maintenance releases but that effort is vain in the master branch, affected by way too many changes for it to ever become reality. This doesn't mean we shouldn't even try, however we shouldn't restrict ourselves; when the ABI needs to be broken, so be it.
First, performance isn't hypothetical. You have code which performs, and have measured various metrics to show this, or you don't have data. No voodoo required. Hypothetical isn't even an argument when it comes to predictable devices (like computers). In the case above, you have a non-performance critical function. It's an error function. It passes strings around. Performance cannot be a consideration here.
Second, it depends on where you want to view the boundary. If a change is internal to DPDK (meaning, the application is never expected to interact with that area of code), then it shouldn't be part of ABI and changing it to your hearts content should be fine.
*rant-on*
I see lots of projects which use DPDK, but I only know of three or so projects which don't directly embed a copy of the DPDK source tree. I think this is a side effect of not having a stable ABI - it's not possible to rely on easily upgrading the library, so people just pick a version and never upgrade. That's bad for everyone. It results in fewer contributions, and difficulty getting people to participate. There's too much text like "Install DPDK 1.8.0. With other versions is not guaranted it works properly" that is embedded in downstream users (that comes from an actual project README). Even at DPDK summit in Dublin, one of the most vocal opponents to ABI stability admitted that his company takes the code and never contributes it back. That is really not friendly.
This even has a different side effect - one of market penetration. It's difficult for distributions to support this model. So, there will be one or two packages they might pick up, with one or two versions. Everything else is "roll your own." And when it's more difficult than 'apt-get|yum|dnf|foo install' then users don't use it, and the cycle continues.
Quoted 9 lines
I'll point out that I can take a number of compiled binaries from 20 years ago[0], and run on a modern kernel. I wish I could take a binary linked to DPDK from 5 months ago, and run it with today's DPDK. I believe that *should* be the goal. I am confident it's achievable. But, I recognize it's hard and requires extra effort.
Anyway, apologies if I took some bait here, and sorry that I ranted off on my own.
I'll add a +1 here. The degree to which the community is concerned about ABI stability is reflected in the fact that no two major releases of DPDK have had compatible ABI's. While thats somewhat understandable from a developer perspective (it really doesn't matter if you have a stable ABI if you always use the latest version, in fact its a hinderance to doing your job), its potentially catastrophic from a downstream consumer point of view. No downstream consumer is going to keep up with the latest version of DPDK on a consistent basis (I'm going to guess that there are still users of DPDK 2.2 floating out there). They just embed a copy, get it working, and move on with their lives. Thats seemingly fine for as long as your foot print is small, but the second that you hit a major bug or security issue in an old version and can't just upgrade the DPDK without a major engineering effort, thats going to be a big problem, and users will look elsewhere for a solution that offers both performance and compatibility. XDP and iovisor are already gaining ground on the performance front, and offer that level of upgradability.
Neil
Quoted 3 lines
Quoted 1 line
(end of rant, thanks for reading)
[0]: https://www.complang.tuwien.ac.at/anton/linux-binary-compatibility.html
Re: [PATCH v1 1/7] ethdev: expose flow API error helper
In reply to Aaron ConoleQuoted 57 lines · Aaron Conole
Quoted 46 lines · Adrien Mazarguil
Quoted 33 lines · Thomas Monjalon
05/10/2017 11:49, Adrien Mazarguil:
Quoted 32 lines
rte_flow_error_set()is a convenient helper to initialize error objects.Since there is no fundamental reason to prevent applications from using it, expose it through the public interface after modifying its return value from positive to negative. This is done for consistency with the rest of the public interface.
Documentation is updated accordingly.
Signed-off-by Adrien Mazarguil <[email protected]>---
lib/librte_ether/rte_flow.h +18
-0/** +* Initialize flow error structure. +* +* @param[out] error +* Pointer to flow error structure (may be NULL). +* @param code +* Related error code (rte_errno). +* @param type +* Cause field and error types. +* @param cause +* Object responsible for the error. +* @param message +* Human-readable error message. +* +* @return +* Negative error code (errno value) and rte_errno is set. +*/ +static inline int +rte_flow_error_set(struct rte_flow_error *error,Quoted 8 lines
lib/librte_ether/rte_flow.h int · line 1295
Quoted 4 lines
lib/librte_ether/rte_flow.h +4
-0+int code, + enum rte_flow_error_type type, + const void *cause, + const char *message)When calling this function, the performance is not critical. So it must not be an inline function.
Please move it to the .c file and add it to the .map file.I'll do it as part of moving this patch into my upcoming series for mlx4, since it relies on the new return value.
Quoted 8 lines
Now for the record, I believe that static inlines, particularly short ones that are not expected to be modified any time soon (provided they are defined properly that is) are harmless.
Yes, that means modifying them, depending on what they do, may cause a global ABI breakage of the underlying library which translates to a major version bump, however exactly the same applies to some macros (in particular the
*_MAXones), structure and other type definitions and so on.
Quoted 7 lines
Sure. I think if it's small and there's never a chance for it to change, then it's okay. But, as you point out - they are similar to macros in that they are code directly added to the application. There is never a way to fix a bug or adjust some semantic behavior without requiring a recompile. That's a strong kind of binding. Certainly we would want that communicated as ABI, regardless of which .h/.c file the function exists, yes?
Yes, I entirely agree, I must admit the stated reasons behind my previous message weren't super clear to begin with as I wrote it after some off-list discussion.
My point is that static inlines must not be considered harmful for what they are particularly in a project like DPDK where there is almost always a performance trade-off depending on the use case. They must be considered on a case basis, and not be forbidden outright with no possible discussion.
Quoted 11 lines
Quoted 5 lines
What I'm getting at is, making this function part of the .map file won't help all that much with the goal of reaching a stable ABI. Making it a normal function on the other hand successfully prevents PMDs and applications from considering its use in their data plane (flow rules management from the data plane is likely the next step for
rte_flow).I don't understand this point well. There are two things - committing to a stable ABI, and communicating information about that ABI. Having .map files doesn't do anything for a stable ABI. It merely communicates information. The stability must come from the developer community and maintainers.
Right, nothing against that. I was referring to the fact the .map file in the case of a shared libray helps with ABI breakage by providing a kind of run-time error check that ensures an application compiled against an older version of a symbol is prevented from starting at all, thereby avoiding any adverse effects later on.
However to benefit from that, some sort of symbol is necessary and that's only possible on non-inline functions. Since we agree that this, in itself, doesn't guarantee ABI stability due to other mandatory factors, I think it cannot be the only ammunition to use against inline functions. Everything must be considered.
Quoted 15 lines
Quoted 7 lines
In my opinion, performance (even if hypothetical) matters more than ABI stability, particularly since the current pattern is for the whole ABI to be broken every other release. I think that with DPDK, ABI stability can only be properly guaranteed in stable/maintenance releases but that effort is vain in the master branch, affected by way too many changes for it to ever become reality. This doesn't mean we shouldn't even try, however we shouldn't restrict ourselves; when the ABI needs to be broken, so be it.
First, performance isn't hypothetical. You have code which performs, and have measured various metrics to show this, or you don't have data. No voodoo required. Hypothetical isn't even an argument when it comes to predictable devices (like computers). In the case above, you have a non-performance critical function. It's an error function. It passes strings around. Performance cannot be a consideration here.
Quoted 4 lines
Second, it depends on where you want to view the boundary. If a change is internal to DPDK (meaning, the application is never expected to interact with that area of code), then it shouldn't be part of ABI and changing it to your hearts content should be fine.
For the specific case of this function, it basically initializes a structure. Now that's probably just me, but I believe initializers are a nice thing to have inline so the compiler can optimize them away wherever it thinks it's worth a few CPU cycles, somewhat like optimized memcpy's (that was the possible performance gain I was thinking about).
This function should only change if the target structure changes, the ABI breakage requiring the major ABI version bump in that case comes from the structure change, not from the inline function itself.
You have a point with the fact the main change from the original code is the return value sign update due to bad design. Had it been non-inline, only the resulting symbol version would have changed, not that of the entire ABI.
Fortunately for this specific case again, that function was neither versioned nor part of the public facing API until this commit. It would have been an entirely different story otherwise.
Quoted 14 lines
*rant-on*
I see lots of projects which use DPDK, but I only know of three or so projects which don't directly embed a copy of the DPDK source tree. I think this is a side effect of not having a stable ABI - it's not possible to rely on easily upgrading the library, so people just pick a version and never upgrade. That's bad for everyone. It results in fewer contributions, and difficulty getting people to participate. There's too much text like "Install DPDK 1.8.0. With other versions is not guaranted it works properly" that is embedded in downstream users (that comes from an actual project README). Even at DPDK summit in Dublin, one of the most vocal opponents to ABI stability admitted that his company takes the code and never contributes it back. That is really not friendly.
I believe all the remaining "bare metal" handling code has been removed by now, however the fact DPDK started as a framework to embed applications and not as a shared library to link them against surely didn't help. There's a lot of design leftovers from this era, such as EAL still snatching argv.
Quoted 7 lines
This even has a different side effect - one of market penetration. It's difficult for distributions to support this model. So, there will be one or two packages they might pick up, with one or two versions. Everything else is "roll your own." And when it's more difficult than 'apt-get|yum|dnf|foo install' then users don't use it, and the cycle continues.
Quoted 5 lines
I'll point out that I can take a number of compiled binaries from 20 years ago[0], and run on a modern kernel. I wish I could take a binary linked to DPDK from 5 months ago, and run it with today's DPDK. I believe that *should* be the goal. I am confident it's achievable. But, I recognize it's hard and requires extra effort.
I do understand your points and the reasons behind them. My goal and the way I see things as a PMD developer are likely why we're having this discussion, so I'd like to summarize why the topic came up on my side:
- DPDK is all about performance *because* without much more performance than what can be achieved with a vanilla Linux kernel, it's only a pain to use compared to the much more comfortable (and stable) socket() API. Since no one would use it, there wouldn't be any market for it.
- Every CPU cycle counts. That makes DPDK difficult to use because we cannot get away with a friendly socket()-like API. Almost every aspect can be configured to tailor it to an application needs. This was historically done at compile time through .config options, now through run-time parameters with the unfortunate side effect of causing the number of TX/RX burst functions grow dangerously in many PMDs to maintain the same level of performance.
- ABI concerns mainly apply to shared libraries [1], which happens to be what Linux distributions want to use for very good reasons. The problem is that shared libraries, by their very nature are not as efficient thanks to PIC, relocations and so on. CPU-bound applications may actually prefer to link against static libraries to squeeze a handful more pps; DPDK cannot ignore this as it has a measurable performance impact for those.
Basically ABI stability is good, unless we want more performance on top of more features. There is no end in sight to new features and associated API changes/ABI breakages.
While I don't want to sound pessimistic, I don't think expecting things to settle without killing DPDK is a realistic goal. That's why I was taking maintenance/stable releases as an example; without new features, ABI stability is almost guaranteed. Efforts could be diverted from the pursuit of ABI stability in the master branch to improve maintenance ones.
Quoted 2 lines
Anyway, apologies if I took some bait here, and sorry that I ranted off on my own.
No worries, I needed to vent a bit myself :)
Note, from the above comments it may not look like it but I'm actually concerned about API/ABI stability where applicable, that was one of the reasons behind the design of rte_flow (which can be extended safely without wrecking ABIs in the process). Unfortunately several bad design choices led to the need for the obligatory ABI breakage that will have to occur at some point in the future.
Quoted 3 lines
Quoted 1 line
(end of rant, thanks for reading)
[0]: https://www.complang.tuwien.ac.at/anton/linux-binary-compatibility.html
[1] Applications are pretty much guaranteed to be recompiled against static ones and encounter issues due to API changes first.
Signature · 2 lines
Adrien Mazarguil 6WIND
PATCH 2/7 ethdev: replace flow API object copy function
In reply to Adrien Mazarguilrte_flow_copy() is bound to duplicate flow rule descriptions (attributes, pattern and list of actions, all at once), however applications sometimes need more freedom, for instance the ability to duplicate only one of the underlying objects (a single pattern item or action) or retrieve other properties such as their names.
Instead of adding dedicated functions to handle each possible use case, this patch introduces rte_flow_conv(). This function supports any number of object conversion operations in an extensible manner.
While rte_flow_copy() could be re-implemented through rte_flow_conv(), it is removed without prior notice; it is assumed that this function has no users besides the fail-safe PMD, therefore no effort is made to maintain it.
--- doc/guides/prog_guide/rte_flow.rst | 19 ++ drivers/net/failsafe/failsafe_ether.c | 6 +- drivers/net/failsafe/failsafe_flow.c | 29 ++- drivers/net/failsafe/failsafe_private.h | 4 +- lib/librte_ether/rte_ethdev_version.map | 1 + lib/librte_ether/rte_flow.c | 296 ++++++++++++++++++--------- lib/librte_ether/rte_flow.h | 193 ++++++++++++++--- 7 files changed, 404 insertions(+), 144 deletions(-)
doc/guides/prog_guide/rte_flow.rst +19 -0
@@ -1697,6 +1697,25 @@ This function initializes ``error`` (if non-NULL) with the provided parameters and sets ``rte_errno`` to ``code``. A negative error ``code`` is then returned. +Object conversion +~~~~~~~~~~~~~~~~~ + +.. code-block:: c + + int + rte_flow_conv(enum rte_flow_conv_op op, + void *dst, + size_t size, + const void *src, + struct rte_flow_error *error); + +Convert ``src`` to ``dst`` according to operation ``op``. Possible +operations include: + +- Attributes, pattern item or action duplication. +- Duplication of an entire pattern or list of actions. +- Duplication of a complete flow rule description. + Caveats -------
drivers/net/failsafe/failsafe_ether.c +3 -3
@@ -257,9 +257,9 @@ fs_eth_dev_conf_apply(struct rte_eth_dev *dev,DEBUG ("Creating flow #%" PRIu32, i++); flow->flows[SUB_ID (sdev)] =rte_flow_create (PORT_ID (sdev), - &flow->fd->attr, - flow->fd->items, - flow->fd->actions, + flow->fd.attr, + flow->fd.pattern, + flow->fd.actions, &ferror); ret = rte_errno;if (ret)
drivers/net/failsafe/failsafe_flow.c +22 -7
@@ -31,8 +31,11 @@* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ +#include <stddef.h> +#include <string.h> #include <sys/queue.h> +#include <rte_errno.h> #include <rte_malloc.h> #include <rte_tailq.h> #include <rte_flow.h>
@@ -46,19 +49,31 @@ fs_flow_allocate(const struct rte_flow_attr *attr, const struct rte_flow_action *actions) {struct rte_flow *flow; -size_t fdsz; +const struct rte_flow_conv_rule rule = { + { attr }, { items }, { actions }, + }; +struct rte_flow_error error; +int ret; - fdsz =rte_flow_copy (NULL ,0 , attr, items, actions); - flow =rte_zmalloc (NULL , -sizeof (struct rte_flow ) + fdsz, + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE ,NULL ,0 , &rule, &error); +if (ret <0 ) { +ERROR ("Unable to compute flow description size (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno)); +return NULL ; + } + flow =rte_zmalloc (NULL , offsetof(struct rte_flow , fd) + ret,RTE_CACHE_LINE_SIZE );if (flow ==NULL ) {ERROR ("Could not allocate new flow" );return NULL ; } - flow->fd = (void *)((uintptr_t )flow +sizeof (*flow)); -if (rte_flow_copy (flow->fd, fdsz, attr, items, actions) != fdsz) { -ERROR ("Failed to copy flow description" ); + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE , &flow->fd, ret, &rule, + &error); +if (ret <0 ) { +ERROR ("Failed to copy flow description (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno));rte_free (flow);return NULL ; }
drivers/net/failsafe/failsafe_private.h +3 -1
@@ -40,6 +40,7 @@#include <rte_dev.h> #include <rte_ethdev.h> #include <rte_devargs.h> +#include <rte_flow.h> #define FAILSAFE_DRIVER_NAME "Fail-safe PMD"
@@ -82,7 +83,8 @@ struct rte_flow {/* sub_flows */ struct rte_flow *flows[FAILSAFE_MAX_ETHPORTS ];/* flow description for synchronization */ -struct rte_flow_desc *fd; +struct rte_flow_conv_rule fd; +uint8_t fd_data[]; };enum dev_state {
lib/librte_ether/rte_ethdev_version.map +1 -0
@@ -192,5 +192,6 @@ DPDK_17.11 { global: rte_eth_dev_reset; + rte_flow_conv; } DPDK_17.08;
lib/librte_ether/rte_flow.c +196 -100
306 lines not on this page · raw message
lib/librte_ether/rte_flow.h +160 -33
217 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH 3/7 ethdev: add flow API item/action name conversion
In reply to Adrien MazarguilThis provides a means for applications to retrieve the name of flow pattern items and actions.
--- doc/guides/prog_guide/rte_flow.rst | 1 + lib/librte_ether/rte_flow.c | 62 +++++++++++++++++++++++++++++++++ lib/librte_ether/rte_flow.h | 52 +++++++++++++++++++++++++++ 3 files changed, 115 insertions(+)
doc/guides/prog_guide/rte_flow.rst +1 -0
@@ -1715,6 +1715,7 @@ operations include: - Attributes, pattern item or action duplication. - Duplication of an entire pattern or list of actions. - Duplication of a complete flow rule description. +- Pattern item or action name retrieval. Caveats -------
lib/librte_ether/rte_flow.c +62 -0
74 lines not on this page · raw message
lib/librte_ether/rte_flow.h +52 -0
58 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH 4/7 app/testpmd: rely on flow API conversion function
In reply to Adrien MazarguilThis commit replaces all local information about pattern items and actions as well as flow rule duplication code with calls to rte_flow_conv().
--- app/test-pmd/config.c | 293 ++++++++++---------------------------------- app/test-pmd/testpmd.h | 7 +- 2 files changed, 66 insertions(+), 234 deletions(-)
app/test-pmd/config.c +64 -229
@@ -918,213 +918,36 @@ port_mtu_set(portid_t port_id, uint16_t mtu)/* Generic flow management functions. */ -/** Generate flow_item[] entry. */ -#define MK_FLOW_ITEM (t, s) \ - [RTE_FLOW_ITEM_TYPE_ ## t] = { \ - .name = # t, \ - .size = s, \ - } - -/** Information about known flow pattern items. */ -static const struct { -const char *name; -size_t size; -} flow_item[] = { -MK_FLOW_ITEM (END ,0 ), -MK_FLOW_ITEM (VOID ,0 ), -MK_FLOW_ITEM (INVERT ,0 ), -MK_FLOW_ITEM (ANY ,sizeof (struct rte_flow_item_any )), -MK_FLOW_ITEM (PF ,0 ), -MK_FLOW_ITEM (VF ,sizeof (struct rte_flow_item_vf )), -MK_FLOW_ITEM (PORT ,sizeof (struct rte_flow_item_port )), -MK_FLOW_ITEM (RAW ,sizeof (struct rte_flow_item_raw )),/* +pattern[] */ -MK_FLOW_ITEM (ETH ,sizeof (struct rte_flow_item_eth )), -MK_FLOW_ITEM (VLAN ,sizeof (struct rte_flow_item_vlan )), -MK_FLOW_ITEM (IPV4 ,sizeof (struct rte_flow_item_ipv4 )), -MK_FLOW_ITEM (IPV6 ,sizeof (struct rte_flow_item_ipv6 )), -MK_FLOW_ITEM (ICMP ,sizeof (struct rte_flow_item_icmp )), -MK_FLOW_ITEM (UDP ,sizeof (struct rte_flow_item_udp )), -MK_FLOW_ITEM (TCP ,sizeof (struct rte_flow_item_tcp )), -MK_FLOW_ITEM (SCTP ,sizeof (struct rte_flow_item_sctp )), -MK_FLOW_ITEM (VXLAN ,sizeof (struct rte_flow_item_vxlan )), -MK_FLOW_ITEM (E_TAG ,sizeof (struct rte_flow_item_e_tag )), -MK_FLOW_ITEM (NVGRE ,sizeof (struct rte_flow_item_nvgre )), -MK_FLOW_ITEM (MPLS ,sizeof (struct rte_flow_item_mpls )), -MK_FLOW_ITEM (GRE ,sizeof (struct rte_flow_item_gre )), -MK_FLOW_ITEM (FUZZY ,sizeof (struct rte_flow_item_fuzzy )), -}; - -/** Compute storage space needed by item specification. */ -static void -flow_item_spec_size (const struct rte_flow_item *item, -size_t *size,size_t *pad) -{ -if (!item->spec) { - *size =0 ; - goto empty; - } -switch (item->type) { -union { -const struct rte_flow_item_raw *raw; - } spec; - -case RTE_FLOW_ITEM_TYPE_RAW : - spec.raw = item->spec; - *size = offsetof(struct rte_flow_item_raw , pattern) + - spec.raw->length *sizeof (*spec.raw->pattern); -break ; -default : - *size = flow_item[item->type].size; -break ; - } -empty: - *pad =RTE_ALIGN_CEIL (*size,sizeof (double )) - *size; -} - -/** Generate flow_action[] entry. */ -#define MK_FLOW_ACTION (t, s) \ - [RTE_FLOW_ACTION_TYPE_ ## t] = { \ - .name = # t, \ - .size = s, \ - } - -/** Information about known flow actions. */ -static const struct { -const char *name; -size_t size; -} flow_action[] = { -MK_FLOW_ACTION (END ,0 ), -MK_FLOW_ACTION (VOID ,0 ), -MK_FLOW_ACTION (PASSTHRU ,0 ), -MK_FLOW_ACTION (MARK ,sizeof (struct rte_flow_action_mark )), -MK_FLOW_ACTION (FLAG ,0 ), -MK_FLOW_ACTION (QUEUE ,sizeof (struct rte_flow_action_queue )), -MK_FLOW_ACTION (DROP ,0 ), -MK_FLOW_ACTION (COUNT ,0 ), -MK_FLOW_ACTION (DUP ,sizeof (struct rte_flow_action_dup )), -MK_FLOW_ACTION (RSS ,sizeof (struct rte_flow_action_rss )),/* +queue[] */ -MK_FLOW_ACTION (PF ,0 ), -MK_FLOW_ACTION (VF ,sizeof (struct rte_flow_action_vf )), -}; - -/** Compute storage space needed by action configuration. */ -static void -flow_action_conf_size (const struct rte_flow_action *action, -size_t *size,size_t *pad) -{ -if (!action->conf) { - *size =0 ; - goto empty; - } -switch (action->type) { -union { -const struct rte_flow_action_rss *rss; - } conf; - -case RTE_FLOW_ACTION_TYPE_RSS : - conf.rss = action->conf; - *size = offsetof(struct rte_flow_action_rss , queue) + - conf.rss->num *sizeof (*conf.rss->queue); -break ; -default : - *size = flow_action[action->type].size; -break ; - } -empty: - *pad =RTE_ALIGN_CEIL (*size,sizeof (double )) - *size; -} -/** Generate a port_flow entry from attributes/pattern/actions. */ static struct port_flow *port_flow_new (const struct rte_flow_attr *attr,const struct rte_flow_item *pattern, -const struct rte_flow_action *actions) +const struct rte_flow_action *actions, +struct rte_flow_error *error) { -const struct rte_flow_item *item; -const struct rte_flow_action *action; +const struct rte_flow_conv_rule rule = { + { attr }, { pattern }, { actions }, + };struct port_flow *pf =NULL ; -size_t tmp; -size_t pad; -size_t off1 =0 ; -size_t off2 =0 ; -int err =ENOTSUP ; - -store: - item = pattern; -if (pf) - pf->pattern = (void *)&pf->data[off1]; -do { -struct rte_flow_item *dst =NULL ; - -if ((unsigned int )item->type >=RTE_DIM (flow_item) || - !flow_item[item->type].name) - goto notsup; -if (pf) - dst =memcpy (pf->data + off1, item,sizeof (*item)); - off1 +=sizeof (*item); -flow_item_spec_size (item, &tmp, &pad); -if (item->spec) { -if (pf) - dst->spec =memcpy (pf->data + off2, - item->spec, tmp); - off2 += tmp + pad; - } -if (item->last) { -if (pf) - dst->last =memcpy (pf->data + off2, - item->last, tmp); - off2 += tmp + pad; - } -if (item->mask) { -if (pf) - dst->mask =memcpy (pf->data + off2, - item->mask, tmp); - off2 += tmp + pad; - } - off2 =RTE_ALIGN_CEIL (off2,sizeof (double )); - }while ((item++)->type !=RTE_FLOW_ITEM_TYPE_END ); - off1 =RTE_ALIGN_CEIL (off1,sizeof (double )); - action = actions; -if (pf) - pf->actions = (void *)&pf->data[off1]; -do { -struct rte_flow_action *dst =NULL ; - -if ((unsigned int )action->type >=RTE_DIM (flow_action) || - !flow_action[action->type].name) - goto notsup; -if (pf) - dst =memcpy (pf->data + off1, action,sizeof (*action)); - off1 +=sizeof (*action); -flow_action_conf_size (action, &tmp, &pad); -if (action->conf) { -if (pf) - dst->conf =memcpy (pf->data + off2, - action->conf, tmp); - off2 += tmp + pad; - } - off2 =RTE_ALIGN_CEIL (off2,sizeof (double )); - }while ((action++)->type !=RTE_FLOW_ACTION_TYPE_END ); -if (pf !=NULL ) -return pf; - off1 =RTE_ALIGN_CEIL (off1,sizeof (double )); - tmp =RTE_ALIGN_CEIL (offsetof(struct port_flow , data),sizeof (double )); - pf =calloc (1 , tmp + off1 + off2); -if (pf ==NULL ) - err = errno; -else { - *pf = (const struct port_flow ){ - .size = tmp + off1 + off2, - .attr = *attr, - }; - tmp -= offsetof(struct port_flow , data); - off2 = tmp + off1; - off1 = tmp; - goto store; - } -notsup: - rte_errno = err; -return NULL ; +int ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE , + &pf->rule,0 , &rule, error); + +if (ret <0 ) +return NULL ; + pf =calloc (1 , offsetof(struct port_flow , rule) + ret); +if (!pf) { +rte_flow_error_set (error, errno, +RTE_FLOW_ERROR_TYPE_UNSPECIFIED ,NULL , +"calloc() failed" ); +return NULL ; + } + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE , + &pf->rule, ret, &rule, error); +if (ret <0 ) { +free (pf); +return NULL ; + } +return pf; }/** Print a message out of a flow error. */
@@ -1208,13 +1031,10 @@ port_flow_create(portid_t port_id, id = port->flow_list->id +1 ; }else id =0 ; - pf =port_flow_new (attr, pattern, actions); + pf =port_flow_new (attr, pattern, actions, &error);if (!pf) { -int err = rte_errno; - -printf ("Cannot allocate flow: %s\n" ,rte_strerror (err));rte_flow_destroy (port_id, flow,NULL ); -return -err; +return port_flow_complain (&error); } pf->next = port->flow_list; pf->id = id;
@@ -1306,6 +1126,7 @@ port_flow_query(portid_t port_id, uint32_t rule,union {struct rte_flow_query_count count; } query; +int ret;if (port_id_is_invalid (port_id,ENABLED_WARN ) || port_id == (portid_t )RTE_PORT_ALL )
@@ -1318,11 +1139,10 @@ port_flow_query(portid_t port_id, uint32_t rule,printf ("Flow rule #%u not found\n" , rule);return -ENOENT ; } -if ((unsigned int )action >=RTE_DIM (flow_action) || - !flow_action[action].name) - name ="unknown" ; -else - name = flow_action[action].name; + ret =rte_flow_conv (RTE_FLOW_CONV_OP_ACTION_NAME_PTR , + &name,sizeof (name), (void *)action, &error); +if (ret <0 ) +return port_flow_complain (&error);switch (action) {case RTE_FLOW_ACTION_TYPE_COUNT :break ;
@@ -1374,47 +1194,62 @@ port_flow_list(portid_t port_id, uint32_t n, const uint32_t group[n])/* Sort flows by group, priority and ID. */ for (pf = port->flow_list; pf !=NULL ; pf = pf->next) {struct port_flow **tmp; +const struct rte_flow_attr *curr = pf->rule.attr;if (n) {/* Filter out unwanted groups. */ for (i =0 ; i != n; ++i) -if (pf->attr.group == group[i]) +if (curr->group == group[i])break ;if (i == n)continue ; } - tmp = &list; -while (*tmp && - (pf->attr.group > (*tmp)->attr.group || - (pf->attr.group == (*tmp)->attr.group && - pf->attr.priority > (*tmp)->attr.priority) || - (pf->attr.group == (*tmp)->attr.group && - pf->attr.priority == (*tmp)->attr.priority && - pf->id > (*tmp)->id))) - tmp = &(*tmp)->tmp; +for (tmp = &list; *tmp; tmp = &(*tmp)->tmp) { +const struct rte_flow_attr *comp = (*tmp)->rule.attr; + +if (curr->group > comp->group || + (curr->group == comp->group && + curr->priority > comp->priority) || + (curr->group == comp->group && + curr->priority == comp->priority && + pf->id > (*tmp)->id)) +continue ; +break ; + } pf->tmp = *tmp; *tmp = pf; }printf ("ID\tGroup\tPrio\tAttr\tRule\n" );for (pf = list; pf !=NULL ; pf = pf->tmp) { -const struct rte_flow_item *item = pf->pattern; -const struct rte_flow_action *action = pf->actions; +const struct rte_flow_item *item = pf->rule.pattern; +const struct rte_flow_action *action = pf->rule.actions; +const char *name;printf ("%" PRIu32"\t%" PRIu32"\t%" PRIu32"\t%c%c\t" , pf->id, - pf->attr.group, - pf->attr.priority, - pf->attr.ingress ?'i' :'-' , - pf->attr.egress ?'e' :'-' ); + pf->rule.attr->group, + pf->rule.attr->priority, + pf->rule.attr->ingress ?'i' :'-' , + pf->rule.attr->egress ?'e' :'-' );while (item->type !=RTE_FLOW_ITEM_TYPE_END ) { +if (rte_flow_conv (RTE_FLOW_CONV_OP_ITEM_NAME_PTR , + &name,sizeof (name), + (void *)(uintptr_t )item->type, +NULL ) <=0 ) + name ="[UNKNOWN]" ;if (item->type !=RTE_FLOW_ITEM_TYPE_VOID ) -printf ("%s " , flow_item[item->type].name); +printf ("%s " , name); ++item; }printf ("=>" );while (action->type !=RTE_FLOW_ACTION_TYPE_END ) { +if (rte_flow_conv (RTE_FLOW_CONV_OP_ACTION_NAME_PTR , + &name,sizeof (name), + (void *)(uintptr_t )action->type, +NULL ) <=0 ) + name ="[UNKNOWN]" ;if (action->type !=RTE_FLOW_ACTION_TYPE_VOID ) -printf (" %s" , flow_action[action->type].name); +printf (" %s" , name); ++action; }printf ("\n" );
app/test-pmd/testpmd.h +2 -5
@@ -151,15 +151,12 @@ struct fwd_stream {/** Descriptor for a single flow. */ struct port_flow { -size_t size;/**< Allocated space including data[]. */ struct port_flow *next;/**< Next flow in list. */ struct port_flow *tmp;/**< Temporary linking. */ uint32_t id;/**< Flow rule ID. */ struct rte_flow *flow;/**< Opaque flow object returned by PMD. */ -struct rte_flow_attr attr;/**< Attributes. */ -struct rte_flow_item *pattern;/**< Pattern. */ -struct rte_flow_action *actions;/**< Actions. */ -uint8_t data[];/**< Storage for pattern/actions. */ +struct rte_flow_conv_rule rule;/* Saved flow rule description. */ +uint8_t data[];/**< Storage for flow rule description */ };/**
Signature · 1 line
2.1.4
PATCH 5/7 ethdev: enhance flow API item/action descriptions
In reply to Adrien MazarguilEnhance description structure with information about embedded flexible arrays in order to handle items and actions through a common function without any dedicated code for special cases.
This commit also moves descriptions to a separate header file for clarity.
--- lib/librte_ether/rte_flow.c | 183 ++++++------------- lib/librte_ether/rte_flow_conv.h | 333 ++++++++++++++++++++++++++++++++++ 2 files changed, 392 insertions(+), 124 deletions(-)
lib/librte_ether/rte_flow.c +59 -124
@@ -37,75 +37,14 @@#include <string.h> #include <rte_common.h> +#include <rte_debug.h> #include <rte_errno.h> #include <rte_branch_prediction.h> #include "rte_ethdev.h" +#include "rte_flow_conv.h" #include "rte_flow_driver.h" #include "rte_flow.h" -/** -* Flow elements description tables. -*/ -struct rte_flow_desc_data { -const char *name; -size_t size; -}; - -/** Generate flow_item[] entry. */ -#define MK_FLOW_ITEM (t, s) \ - [RTE_FLOW_ITEM_TYPE_ ## t] = { \ - .name = # t, \ - .size = s, \ - } - -/** Information about known flow pattern items. */ -static const struct rte_flow_desc_data rte_flow_desc_item[] = { -MK_FLOW_ITEM (END ,0 ), -MK_FLOW_ITEM (VOID ,0 ), -MK_FLOW_ITEM (INVERT ,0 ), -MK_FLOW_ITEM (ANY ,sizeof (struct rte_flow_item_any )), -MK_FLOW_ITEM (PF ,0 ), -MK_FLOW_ITEM (VF ,sizeof (struct rte_flow_item_vf )), -MK_FLOW_ITEM (PORT ,sizeof (struct rte_flow_item_port )), -MK_FLOW_ITEM (RAW ,sizeof (struct rte_flow_item_raw )),/* +pattern[] */ -MK_FLOW_ITEM (ETH ,sizeof (struct rte_flow_item_eth )), -MK_FLOW_ITEM (VLAN ,sizeof (struct rte_flow_item_vlan )), -MK_FLOW_ITEM (IPV4 ,sizeof (struct rte_flow_item_ipv4 )), -MK_FLOW_ITEM (IPV6 ,sizeof (struct rte_flow_item_ipv6 )), -MK_FLOW_ITEM (ICMP ,sizeof (struct rte_flow_item_icmp )), -MK_FLOW_ITEM (UDP ,sizeof (struct rte_flow_item_udp )), -MK_FLOW_ITEM (TCP ,sizeof (struct rte_flow_item_tcp )), -MK_FLOW_ITEM (SCTP ,sizeof (struct rte_flow_item_sctp )), -MK_FLOW_ITEM (VXLAN ,sizeof (struct rte_flow_item_vxlan )), -MK_FLOW_ITEM (MPLS ,sizeof (struct rte_flow_item_mpls )), -MK_FLOW_ITEM (GRE ,sizeof (struct rte_flow_item_gre )), -MK_FLOW_ITEM (E_TAG ,sizeof (struct rte_flow_item_e_tag )), -MK_FLOW_ITEM (NVGRE ,sizeof (struct rte_flow_item_nvgre )), -}; - -/** Generate flow_action[] entry. */ -#define MK_FLOW_ACTION (t, s) \ - [RTE_FLOW_ACTION_TYPE_ ## t] = { \ - .name = # t, \ - .size = s, \ - } - -/** Information about known flow actions. */ -static const struct rte_flow_desc_data rte_flow_desc_action[] = { -MK_FLOW_ACTION (END ,0 ), -MK_FLOW_ACTION (VOID ,0 ), -MK_FLOW_ACTION (PASSTHRU ,0 ), -MK_FLOW_ACTION (MARK ,sizeof (struct rte_flow_action_mark )), -MK_FLOW_ACTION (FLAG ,0 ), -MK_FLOW_ACTION (QUEUE ,sizeof (struct rte_flow_action_queue )), -MK_FLOW_ACTION (DROP ,0 ), -MK_FLOW_ACTION (COUNT ,0 ), -MK_FLOW_ACTION (DUP ,sizeof (struct rte_flow_action_dup )), -MK_FLOW_ACTION (RSS ,sizeof (struct rte_flow_action_rss )),/* +queue[] */ -MK_FLOW_ACTION (PF ,0 ), -MK_FLOW_ACTION (VF ,sizeof (struct rte_flow_action_vf )), -}; -/* Get generic flow operations structure from a port. */ const struct rte_flow_ops *rte_flow_ops_get (uint8_t port_id,struct rte_flow_error *error)
@@ -243,59 +182,39 @@ rte_flow_isolate(uint8_t port_id, NULL, rte_strerror(ENOSYS)); } -/** Compute storage space needed by item specification. */ -static void -flow_item_spec_size (const struct rte_flow_item *item, -size_t *size,size_t *pad) -{ -if (!item->spec) { - *size =0 ; - goto empty; - } -switch (item->type) { -union { -const struct rte_flow_item_raw *raw; - } spec; - -/* Not a fall-through */ -case RTE_FLOW_ITEM_TYPE_RAW : - spec.raw = item->spec; - *size = offsetof(struct rte_flow_item_raw , pattern) + - spec.raw->length *sizeof (*spec.raw->pattern); -break ; -default : - *size = rte_flow_desc_item[item->type].size; -break ; - } -empty: - *pad =RTE_ALIGN_CEIL (*size,sizeof (double )) - *size; -} - -/** Compute storage space needed by action configuration. */ +/** Compute storage space needed for item->spec or action->conf. */ static void -flow_action_conf_size (const struct rte_flow_action *action, -size_t *size,size_t *pad) +rte_flow_conv_res_size (const struct rte_flow_conv_res *res,const void *obj, +size_t *size,size_t *pad) { -if (!action->conf) { +if (!obj) { *size =0 ; - goto empty; - } -switch (action->type) { -union { -const struct rte_flow_action_rss *rss; - } conf; - -/* Not a fall-through. */ -case RTE_FLOW_ACTION_TYPE_RSS : - conf.rss = action->conf; - *size = offsetof(struct rte_flow_action_rss , queue) + - conf.rss->num *sizeof (*conf.rss->queue); -break ; -default : - *size = rte_flow_desc_action[action->type].size; -break ; + }else if (res->flex_off) { +void *flex_len_ptr = + (void *)((uintptr_t )obj + res->flex_len_off); +uint64_t flex_len =0 ; + +switch (res->flex_len_type) { +case sizeof (uint8_t ): + flex_len = *(uint8_t *)flex_len_ptr; +break ; +case sizeof (uint16_t ): + flex_len = *(uint16_t *)flex_len_ptr; +break ; +case sizeof (uint32_t ): + flex_len = *(uint32_t *)flex_len_ptr; +break ; +case sizeof (uint64_t ): + flex_len = *(uint64_t *)flex_len_ptr; +break ; +default : +RTE_ASSERT (!"unsupported flex_len_type in" +" struct rte_flow_conv_res" ); + } + *size = res->flex_off + res->flex_elt_size * flex_len; + }else { + *size = res->size; } -empty: *pad =RTE_ALIGN_CEIL (*size,sizeof (double )) - *size; }
@@ -315,15 +234,17 @@ rte_flow_conv_pattern(struct rte_flow_item *dst, store:for (off =0 , i =0 ; !num || i != num; ++i, ++src, ++dst) { +const struct rte_flow_conv_res *res = + &rte_flow_conv_res_item[src->type];size_t spec;size_t pad; -if ((size_t )src->type >=RTE_DIM (rte_flow_desc_item) || - !rte_flow_desc_item[src->type].name) +if ((size_t )src->type >=RTE_DIM (rte_flow_conv_res_item) || + !res->name) goto notsup;if (store) *dst = (struct rte_flow_item ){ .type = src->type, }; -flow_item_spec_size (src, &spec, &pad); +rte_flow_conv_res_size (res, src->spec, &spec, &pad);if (spec) off =RTE_ALIGN_CEIL (off,sizeof (double ));if (src->spec) {
@@ -380,15 +301,17 @@ rte_flow_conv_actions(struct rte_flow_action *dst, store:for (off =0 , i =0 ; !num || i != num; ++i, ++src, ++dst) { +const struct rte_flow_conv_res *res = + &rte_flow_conv_res_action[src->type];size_t conf;size_t pad; -if ((size_t )src->type >=RTE_DIM (rte_flow_desc_action) || - !rte_flow_desc_action[src->type].name) +if ((size_t )src->type >=RTE_DIM (rte_flow_conv_res_action) || + !res->name) goto notsup;if (store) *dst = (struct rte_flow_action ){ .type = src->type, }; -flow_action_conf_size (src, &conf, &pad); +rte_flow_conv_res_size (res, src->conf, &conf, &pad);if (conf) off =RTE_ALIGN_CEIL (off,sizeof (double ));if (store && conf)
@@ -474,11 +397,17 @@ rte_flow_conv_name(int is_action, struct rte_flow_error *error) {const struct { -const struct rte_flow_desc_data *data; +const struct rte_flow_conv_res *data;size_t num; } res_data[2 ] = { - { rte_flow_desc_item,RTE_DIM (rte_flow_desc_item), }, - { rte_flow_desc_action,RTE_DIM (rte_flow_desc_action), }, + { + rte_flow_conv_res_item, +RTE_DIM (rte_flow_conv_res_item), + }, + { + rte_flow_conv_res_action, +RTE_DIM (rte_flow_conv_res_action), + }, }, *const res = &res_data[!!is_action];unsigned int obj_type = (uintptr_t )src;
@@ -498,11 +427,17 @@ rte_flow_conv_name_ptr(int is_action, struct rte_flow_error *error) {const struct { -const struct rte_flow_desc_data *data; +const struct rte_flow_conv_res *data;size_t num; } res_data[2 ] = { - { rte_flow_desc_item,RTE_DIM (rte_flow_desc_item), }, - { rte_flow_desc_action,RTE_DIM (rte_flow_desc_action), }, + { + rte_flow_conv_res_item, +RTE_DIM (rte_flow_conv_res_item), + }, + { + rte_flow_conv_res_action, +RTE_DIM (rte_flow_conv_res_action), + }, }, *const res = &res_data[!!is_action];unsigned int obj_type = (uintptr_t )src;
lib/librte_ether/rte_flow_conv.h new file +333 -0
333 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH 6/7 ethdev: generate flow API conversion header
In reply to Adrien MazarguilAdd script and build target to generate rte_flow_conv.h (used by rte_flow_conv()) from rte_flow.h.
Although the resulting header file is internal and not public, this is not done automatically since it is versioned in the source tree.
Developers can update it after making changes to rte_flow.h by running:
make lib/librte_ether_sub RTE_MAKE_SUBTARGET=rte_flow_conv.h
--- MAINTAINERS | 1 + buildtools/gen-rte_flow_conv-h.sh | 264 +++++++++++++++++++++++++++++++++ lib/librte_ether/Makefile | 10 ++ 3 files changed, 275 insertions(+)
MAINTAINERS +1 -0
@@ -253,6 +253,7 @@ F: devtools/test-null.sh Flow API M: Adrien Mazarguil <[email protected]> +F: buildtools/gen-rte_flow_conv-h.sh F: lib/librte_ether/rte_flow* Traffic Management API - EXPERIMENTAL
buildtools/gen-rte_flow_conv-h.sh new file +264 -0
264 lines not on this page · raw message
lib/librte_ether/Makefile +10 -0
@@ -61,4 +61,14 @@ SYMLINK-y-include += rte_flow_driver.h SYMLINK-y-include += rte_tm.h SYMLINK-y-include += rte_tm_driver.h +# +# This file is versioned thus not built automatically. +# To re-generate it, run: +# +# make lib/librte_ether_sub RTE_MAKE_SUBTARGET=rte_flow_conv.h +# +rte_flow_conv.h: rte_flow.h + @$(if $V,,echo " GEN $(@F)") + $Q sh -- $(RTE_SDK)/buildtools/gen-rte_flow_conv-h.sh "$(<D)/$(@F)" + include $(RTE_SDK)/mk/rte.lib.mk
Signature · 1 line
2.1.4
PATCH 7/7 ethdev: update flow API conversion header
In reply to Adrien MazarguilSynchronize rte_flow_conv() capabilities with rte_flow.h.
--- lib/librte_ether/rte_flow_conv.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
lib/librte_ether/rte_flow_conv.h +12 -0
@@ -35,6 +35,10 @@* RTE generic flow API (internal) * * This file exports resources needed by rte_flow_conv(). +* +* DO NOT EDIT THIS FILE. +* +* It was generated from rte_flow.h, run gen-rte_flow_conv-h.sh to update it. */ #ifndef RTE_FLOW_CONV_H_
@@ -228,6 +232,14 @@ static const struct rte_flow_conv_res rte_flow_conv_res_item[] = { .flex_elt_size = 0, .flex_off = 0, }, + [RTE_FLOW_ITEM_TYPE_FUZZY] = { + .name = "fuzzy", + .size = sizeof(struct rte_flow_item_fuzzy), + .flex_len_type = 0, + .flex_len_off = 0, + .flex_elt_size = 0, + .flex_off = 0, + }, };/** Actions description table. */
Signature · 1 line
2.1.4
Quoted 24 lines · Adrien Mazarguil
This series brings enhancements to various
rte_flowhelpers:- Allow applications to use
rte_flow_error_set()by making it part of the public interface and documenting it as such.- Address rte_flow_copy()'s limitations by replacing it with the more versatile
rte_flow_conv(). This new function allows retrieving other properties such as item/action names, enabling testpmd to finally use it and get rid of duplicated code.- Add a script (
gen-rte_flow_conv-h.sh) to help with generating the resources used byrte_flow_conv(). Developers should run it when adding or modifying pattern items or actions (done as part of this series to add the missing "fuzzy" pattern item).- Future plans for
rte_flow_conv()include translating error codes to human-readable messages, so applications do not have to make their own.All these changes address concerns raised a couple of months ago [1]. Work on these patches actually started at the time but I was unable to complete and clean them up until recently.
Quoted 8 lines
Adrien Mazarguil (7):
ethdev: expose flow API error helper
ethdev: replace flow API object copy function
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function
ethdev: enhance flow API item/action descriptions
ethdev: generate flow API conversion header
ethdev: update flow API conversion header
Hi Adrien,
This received too late for this release cycle, and changes in rte_flow library may effect PMDs.
I suggest deferring the set to next release, what do you think?
Thanks,
ferruh
Quoted 34 lines · Ferruh Yigit
Quoted 24 lines · Adrien Mazarguil
This series brings enhancements to various
rte_flowhelpers:- Allow applications to use
rte_flow_error_set()by making it part of the public interface and documenting it as such.- Address rte_flow_copy()'s limitations by replacing it with the more versatile
rte_flow_conv(). This new function allows retrieving other properties such as item/action names, enabling testpmd to finally use it and get rid of duplicated code.- Add a script (
gen-rte_flow_conv-h.sh) to help with generating the resources used byrte_flow_conv(). Developers should run it when adding or modifying pattern items or actions (done as part of this series to add the missing "fuzzy" pattern item).- Future plans for
rte_flow_conv()include translating error codes to human-readable messages, so applications do not have to make their own.All these changes address concerns raised a couple of months ago [1]. Work on these patches actually started at the time but I was unable to complete and clean them up until recently.
Quoted 8 lines
Adrien Mazarguil (7):
ethdev: expose flow API error helper
ethdev: replace flow API object copy function
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function
ethdev: enhance flow API item/action descriptions
ethdev: generate flow API conversion header
ethdev: update flow API conversion header
Quoted 6 lines
Hi Adrien,
This received too late for this release cycle, and changes in
rte_flowlibrary may effect PMDs.I suggest deferring the set to next release, what do you think?
Hi Ferruh,
My opinion as the author (since you're asking :) is that it would be nice to have it in this release assuming reviewers don't find blocker issues with it.
To summarize the changes from a PMD standpoint:
- rte_flow_error() (previously not public) switching from positive to negative return value like the other rte_flow_*() functions. The only PMDs relying on its return value so far are mlx4 and tap.
- rte_flow_copy() disappearing. This function was temporary pending a better solution, and so far is only used by fail-safe PMD (modified as part of this series). Besides fail-safe, PMDs did not a have a use case for this function.
These patches were originally targeted at 17.08, and since the "fuzzy" item is missing from rte_flow_copy() (GTP/GTPU/GTPC are now also missing by the way) and there is currently a lot of redundancy between this function and testpmd's internals, I thought it would be a good time to have everything in a single place. I was also considering using rte_flow_conv() in upcoming mlx4 patches in case it was included.
So here's my suggestion: I can track all rte_flow-related changes in PMDs and in rte_flow itself and update this series accordingly until things have settled (e.g. I'll re-submit to rebase and include GTP). Once applied, I will check all new code that relies on these two functions and update it if necessary until the release. How about that?
Signature · 2 lines
Adrien Mazarguil 6WIND
Quoted 40 lines · Adrien Mazarguil
Quoted 39 lines · Ferruh Yigit
Quoted 24 lines · Adrien Mazarguil
This series brings enhancements to various
rte_flowhelpers:- Allow applications to use
rte_flow_error_set()by making it part of the public interface and documenting it as such.- Address rte_flow_copy()'s limitations by replacing it with the more versatile
rte_flow_conv(). This new function allows retrieving other properties such as item/action names, enabling testpmd to finally use it and get rid of duplicated code.- Add a script (
gen-rte_flow_conv-h.sh) to help with generating the resources used byrte_flow_conv(). Developers should run it when adding or modifying pattern items or actions (done as part of this series to add the missing "fuzzy" pattern item).- Future plans for
rte_flow_conv()include translating error codes to human-readable messages, so applications do not have to make their own.All these changes address concerns raised a couple of months ago [1]. Work on these patches actually started at the time but I was unable to complete and clean them up until recently.
Quoted 8 lines
Adrien Mazarguil (7):
ethdev: expose flow API error helper
ethdev: replace flow API object copy function
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function
ethdev: enhance flow API item/action descriptions
ethdev: generate flow API conversion header
ethdev: update flow API conversion headerHi Adrien,
This received too late for this release cycle, and changes in
rte_flowlibrary may effect PMDs.
Quoted 7 lines
Quoted 1 line
I suggest deferring the set to next release, what do you think?
Hi Ferruh,
My opinion as the author (since you're asking :) is that it would be nice to have it in this release assuming reviewers don't find blocker issues with it.
Review part may be the problem, since this is very short notice before release, relevant parties may not review this on time.
And they will be right to not expect a new feature like this after proposal deadline.
Quoted 11 lines
To summarize the changes from a PMD standpoint:
-
rte_flow_error()(previously not public) switching from positive to negative return value like the other rte_flow_*() functions. The only PMDs relying on its return value so far are mlx4 and tap.-
rte_flow_copy()disappearing. This function was temporary pending a better solution, and so far is only used by fail-safe PMD (modified as part of this series). Besides fail-safe, PMDs did not a have a use case for this function.
Although you update all rte_flow_copy() usage in the DPDK, this is public API right, and technically a user code may be using this, can we remove this without notice?
Quoted 8 lines
These patches were originally targeted at 17.08, and since the "fuzzy" item is missing from
rte_flow_copy()(GTP/GTPU/GTPC are now also missing by the way) and there is currently a lot of redundancy between this function and testpmd's internals, I thought it would be a good time to have everything in a single place. I was also considering usingrte_flow_conv()in upcoming mlx4 patches in case it was included.
Quoted 5 lines
So here's my suggestion: I can track all rte_flow-related changes in PMDs and in
rte_flowitself and update this series accordingly until things have settled (e.g. I'll re-submit to rebase and include GTP). Once applied, I will check all new code that relies on these two functions and update it if necessary until the release. How about that?
I have no concern that you will do the necessary updates, and agreed it is good to get updates to help maintenance, and it would be nice to have these in the this LTS release.
After above said, API changes one week before integration deadline, a new script and make target for automated header file, I am a little scared :), I will be much relieved to get this in the beginning of the next release cycle.
I would like to see more comment on this, specially from PMD maintainers.
Quoted 45 lines · Ferruh Yigit
Quoted 44 lines · Adrien Mazarguil
Quoted 34 lines · Ferruh Yigit
Quoted 24 lines · Adrien Mazarguil
This series brings enhancements to various
rte_flowhelpers:- Allow applications to use
rte_flow_error_set()by making it part of the public interface and documenting it as such.- Address rte_flow_copy()'s limitations by replacing it with the more versatile
rte_flow_conv(). This new function allows retrieving other properties such as item/action names, enabling testpmd to finally use it and get rid of duplicated code.- Add a script (
gen-rte_flow_conv-h.sh) to help with generating the resources used byrte_flow_conv(). Developers should run it when adding or modifying pattern items or actions (done as part of this series to add the missing "fuzzy" pattern item).- Future plans for
rte_flow_conv()include translating error codes to human-readable messages, so applications do not have to make their own.All these changes address concerns raised a couple of months ago [1]. Work on these patches actually started at the time but I was unable to complete and clean them up until recently.
Quoted 8 lines
Adrien Mazarguil (7):
ethdev: expose flow API error helper
ethdev: replace flow API object copy function
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function
ethdev: enhance flow API item/action descriptions
ethdev: generate flow API conversion header
ethdev: update flow API conversion headerQuoted 6 lines
Hi Adrien,
This received too late for this release cycle, and changes in
rte_flowlibrary may effect PMDs.I suggest deferring the set to next release, what do you think?
Hi Ferruh,
Quoted 8 lines
Quoted 3 lines
My opinion as the author (since you're asking :) is that it would be nice to have it in this release assuming reviewers don't find blocker issues with it.
Review part may be the problem, since this is very short notice before release, relevant parties may not review this on time.
And they will be right to not expect a new feature like this after proposal deadline.
Yes, I generally agree with that.
Quoted 6 lines
Quoted 6 lines
To summarize the changes from a PMD standpoint:
-
rte_flow_error()(previously not public) switching from positive to negative return value like the other rte_flow_*() functions. The only PMDs relying on its return value so far are mlx4 and tap.
Quoted 8 lines
Quoted 4 lines
-
rte_flow_copy()disappearing. This function was temporary pending a better solution, and so far is only used by fail-safe PMD (modified as part of this series). Besides fail-safe, PMDs did not a have a use case for this function.Although you update all
rte_flow_copy()usage in the DPDK, this is public API right, and technically a user code may be using this, can we remove this without notice?
Right, actually rte_flow_copy() was missing the EXPERIMENTAL tag. It's been present for a single release and wasn't documented as being officially part of the public rte_flow interface, unlike this series with rte_flow_conv().
For various reasons including its lack of flexibility (enforced by the struct rte_flow_desc format), I honestly believe rte_flow_copy() is only presently used by the fail-safe PMD. I may be wrong, and in the case of complaints the plan is to re-add rte_flow_copy() as a wrapper to rte_flow_conv() later as a standard maintenance fix.
Quoted 13 lines
Quoted 7 lines
These patches were originally targeted at 17.08, and since the "fuzzy" item is missing from
rte_flow_copy()(GTP/GTPU/GTPC are now also missing by the way) and there is currently a lot of redundancy between this function and testpmd's internals, I thought it would be a good time to have everything in a single place. I was also considering usingrte_flow_conv()in upcoming mlx4 patches in case it was included.Quoted 5 lines
So here's my suggestion: I can track all rte_flow-related changes in PMDs and in
rte_flowitself and update this series accordingly until things have settled (e.g. I'll re-submit to rebase and include GTP). Once applied, I will check all new code that relies on these two functions and update it if necessary until the release. How about that?
Quoted 8 lines
I have no concern that you will do the necessary updates, and agreed it is good to get updates to help maintenance, and it would be nice to have these in the this LTS release.
After above said, API changes one week before integration deadline, a new script and make target for automated header file, I am a little scared :), I will be much relieved to get this in the beginning of the next release cycle.
I can drop the script from this series to speed up inclusion if it there's any concern about it. It's only a helper to update rte_flow_conv.h after modifying rte_flow.h, I thought it could be useful to anyone, hence I've included it but it's pretty much optional.
Quoted 1 line
I would like to see more comment on this, specially from PMD maintainers.
Me too. I don't even mind negative ones!
Here's what I plan to do regardless, seeing most concerns so far are with rte_flow_copy()/rte_flow_conv():
- Whether this series is included for 17.11 or later, a v2 is already necessary.
- I will drop the rte_flow_error() change to submit it instead along another upcoming series for mlx4 where it's the most needed.
- We'll then continue to discuss rte_flow_conv() as a something nice to have but not super urgent to integrate and I'll keep trying to convince everyone it's safe enough.
- Once it becomes clear there's no way to have it for 17.11, I'll update this series as a somewhat late deprecation notice for rte_flow_copy().
Sounds good?
Signature · 2 lines
Adrien Mazarguil 6WIND
Quoted 122 lines · Adrien Mazarguil
Quoted 45 lines · Ferruh Yigit
Quoted 44 lines · Adrien Mazarguil
Quoted 34 lines · Ferruh Yigit
Quoted 24 lines · Adrien Mazarguil
This series brings enhancements to various
rte_flowhelpers:- Allow applications to use
rte_flow_error_set()by making it part of the public interface and documenting it as such.- Address rte_flow_copy()'s limitations by replacing it with the more versatile
rte_flow_conv(). This new function allows retrieving other properties such as item/action names, enabling testpmd to finally use it and get rid of duplicated code.- Add a script (
gen-rte_flow_conv-h.sh) to help with generating the resources used byrte_flow_conv(). Developers should run it when adding or modifying pattern items or actions (done as part of this series to add the missing "fuzzy" pattern item).- Future plans for
rte_flow_conv()include translating error codes to human-readable messages, so applications do not have to make their own.All these changes address concerns raised a couple of months ago [1]. Work on these patches actually started at the time but I was unable to complete and clean them up until recently.
Quoted 8 lines
Adrien Mazarguil (7):
ethdev: expose flow API error helper
ethdev: replace flow API object copy function
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function
ethdev: enhance flow API item/action descriptions
ethdev: generate flow API conversion header
ethdev: update flow API conversion headerQuoted 6 lines
Hi Adrien,
This received too late for this release cycle, and changes in
rte_flowlibrary may effect PMDs.I suggest deferring the set to next release, what do you think?
Hi Ferruh,
Quoted 8 lines
Quoted 3 lines
My opinion as the author (since you're asking :) is that it would be nice to have it in this release assuming reviewers don't find blocker issues with it.
Review part may be the problem, since this is very short notice before release, relevant parties may not review this on time.
And they will be right to not expect a new feature like this after proposal deadline.Yes, I generally agree with that.
Quoted 6 lines
Quoted 6 lines
To summarize the changes from a PMD standpoint:
-
rte_flow_error()(previously not public) switching from positive to negative return value like the other rte_flow_*() functions. The only PMDs relying on its return value so far are mlx4 and tap.Quoted 8 lines
Quoted 4 lines
-
rte_flow_copy()disappearing. This function was temporary pending a better solution, and so far is only used by fail-safe PMD (modified as part of this series). Besides fail-safe, PMDs did not a have a use case for this function.Although you update all
rte_flow_copy()usage in the DPDK, this is public API right, and technically a user code may be using this, can we remove this without notice?Right, actually
rte_flow_copy()was missing the EXPERIMENTAL tag. It's been present for a single release and wasn't documented as being officially part of the publicrte_flowinterface, unlike this series withrte_flow_conv().For various reasons including its lack of flexibility (enforced by the struct
rte_flow_descformat), I honestly believerte_flow_copy()is only presently used by the fail-safe PMD. I may be wrong, and in the case of complaints the plan is to re-addrte_flow_copy()as a wrapper torte_flow_conv()later as a standard maintenance fix.Quoted 13 lines
Quoted 7 lines
These patches were originally targeted at 17.08, and since the "fuzzy" item is missing from
rte_flow_copy()(GTP/GTPU/GTPC are now also missing by the way) and there is currently a lot of redundancy between this function and testpmd's internals, I thought it would be a good time to have everything in a single place. I was also considering usingrte_flow_conv()in upcoming mlx4 patches in case it was included.Quoted 5 lines
So here's my suggestion: I can track all rte_flow-related changes in PMDs and in
rte_flowitself and update this series accordingly until things have settled (e.g. I'll re-submit to rebase and include GTP). Once applied, I will check all new code that relies on these two functions and update it if necessary until the release. How about that?Quoted 8 lines
I have no concern that you will do the necessary updates, and agreed it is good to get updates to help maintenance, and it would be nice to have these in the this LTS release.
After above said, API changes one week before integration deadline, a new script and make target for automated header file, I am a little scared :), I will be much relieved to get this in the beginning of the next release cycle.
I can drop the script from this series to speed up inclusion if it there's any concern about it. It's only a helper to update
rte_flow_conv.hafter modifyingrte_flow.h, I thought it could be useful to anyone, hence I've included it but it's pretty much optional.Quoted 1 line
I would like to see more comment on this, specially from PMD maintainers.
Me too. I don't even mind negative ones!
Here's what I plan to do regardless, seeing most concerns so far are with rte_flow_copy()/rte_flow_conv():
- Whether this series is included for 17.11 or later, a v2 is already necessary.
- I will drop the
rte_flow_error()change to submit it instead along another upcoming series for mlx4 where it's the most needed.
Quoted 8 lines
- We'll then continue to discuss
rte_flow_conv()as a something nice to have but not super urgent to integrate and I'll keep trying to convince everyone it's safe enough.- Once it becomes clear there's no way to have it for 17.11, I'll update this series as a somewhat late deprecation notice for
rte_flow_copy().Sounds good?
OK. Lets get rte_flow_error() change and not block mlx4 changes.
And I still suggest waiting beginning of the release for rest of the patch. So it can come with optional header auto generation.
Related to the rte_flow_error_set() modification, as far as I can see it doesn't effect drivers but following drivers are now using it:
drivers/net/bnxt/drivers/net/e1000/drivers/net/enic/drivers/net/i40e/drivers/net/ixgbe/drivers/net/mlx4/drivers/net/mlx5/drivers/net/sfc/drivers/net/tap/
There is already tap and mlx4 fixes in the patch to fix "return -rte_flow_error_set(...)" kind of usage.
Rest uses rte_flow_error_set() as:
"rte_flow_error_set(...);
return -rte_errno;
"
But now it can directly be used as:
"
return rte_flow_error_set(...);
"
What do you think updating to that usage?
Would you mind updating those drivers as above in a separate patch?
Thanks,
ferruh
Quoted 2 lines · Ferruh Yigit
Quoted 1 line · Adrien Mazarguil
On Tue, Oct 10, 2017 at 07:05:30PM +0100, Ferruh Yigit wrote:
<snip>
Quoted 30 lines
Quoted 30 lines
Quoted 4 lines
After above said, API changes one week before integration deadline, a new script and make target for automated header file, I am a little scared :), I will be much relieved to get this in the beginning of the next release cycle.
I can drop the script from this series to speed up inclusion if it there's any concern about it. It's only a helper to update
rte_flow_conv.hafter modifyingrte_flow.h, I thought it could be useful to anyone, hence I've included it but it's pretty much optional.Quoted 1 line
I would like to see more comment on this, specially from PMD maintainers.
Me too. I don't even mind negative ones!
Here's what I plan to do regardless, seeing most concerns so far are with rte_flow_copy()/rte_flow_conv():
- Whether this series is included for 17.11 or later, a v2 is already necessary.
- I will drop the
rte_flow_error()change to submit it instead along another upcoming series for mlx4 where it's the most needed.- We'll then continue to discuss
rte_flow_conv()as a something nice to have but not super urgent to integrate and I'll keep trying to convince everyone it's safe enough.- Once it becomes clear there's no way to have it for 17.11, I'll update this series as a somewhat late deprecation notice for
rte_flow_copy().
Quoted 6 lines
Quoted 1 line
Sounds good?
OK. Lets get
rte_flow_error()change and not block mlx4 changes.And I still suggest waiting beginning of the release for rest of the patch. So it can come with optional header auto generation.
OK, I will re-submit an updated version later then.
Quoted 23 lines
Related to the
rte_flow_error_set()modification, as far as I can see it doesn't effect drivers but following drivers are now using it:
drivers/net/bnxt/drivers/net/e1000/drivers/net/enic/drivers/net/i40e/drivers/net/ixgbe/drivers/net/mlx4/drivers/net/mlx5/drivers/net/sfc/drivers/net/tap/There is already tap and mlx4 fixes in the patch to fix "return -rte_flow_error_set(...)" kind of usage.
Rest uses
rte_flow_error_set()as:"
rte_flow_error_set(...);
return -rte_errno;
"
Quoted 6 lines
But now it can directly be used as:
"
returnrte_flow_error_set(...);
"What do you think updating to that usage?
Well, that is actually how I originally envisioned this function to be used, but for whatever reason I thought a positive return value was the way to go at the time. Too bad.
Quoted 1 line
Would you mind updating those drivers as above in a separate patch?
That would be nice but I'm not familiar with most of these PMDs (I'm only updating mlx4 as part of the RSS resurrection series), and there's always a risk of breaking something.
The current approach of relying on rte_errno is OK since this function updates it. I'll put it on my TODO list anyway but given its current size, it's going to be low priority.
Signature · 2 lines
Adrien Mazarguil 6WIND
Quoted 77 lines · Adrien Mazarguil
Quoted 2 lines · Ferruh Yigit
Quoted 1 line · Adrien Mazarguil
On Tue, Oct 10, 2017 at 07:05:30PM +0100, Ferruh Yigit wrote:
<snip>
Quoted 30 lines
Quoted 30 lines
Quoted 4 lines
After above said, API changes one week before integration deadline, a new script and make target for automated header file, I am a little scared :), I will be much relieved to get this in the beginning of the next release cycle.
I can drop the script from this series to speed up inclusion if it there's any concern about it. It's only a helper to update
rte_flow_conv.hafter modifyingrte_flow.h, I thought it could be useful to anyone, hence I've included it but it's pretty much optional.Quoted 1 line
I would like to see more comment on this, specially from PMD maintainers.
Me too. I don't even mind negative ones!
Here's what I plan to do regardless, seeing most concerns so far are with rte_flow_copy()/rte_flow_conv():
- Whether this series is included for 17.11 or later, a v2 is already necessary.
- I will drop the
rte_flow_error()change to submit it instead along another upcoming series for mlx4 where it's the most needed.- We'll then continue to discuss
rte_flow_conv()as a something nice to have but not super urgent to integrate and I'll keep trying to convince everyone it's safe enough.- Once it becomes clear there's no way to have it for 17.11, I'll update this series as a somewhat late deprecation notice for
rte_flow_copy().Quoted 6 lines
Quoted 1 line
Sounds good?
OK. Lets get
rte_flow_error()change and not block mlx4 changes.And I still suggest waiting beginning of the release for rest of the patch. So it can come with optional header auto generation.
OK, I will re-submit an updated version later then.
Quoted 23 lines
Related to the
rte_flow_error_set()modification, as far as I can see it doesn't effect drivers but following drivers are now using it:
drivers/net/bnxt/drivers/net/e1000/drivers/net/enic/drivers/net/i40e/drivers/net/ixgbe/drivers/net/mlx4/drivers/net/mlx5/drivers/net/sfc/drivers/net/tap/There is already tap and mlx4 fixes in the patch to fix "return -rte_flow_error_set(...)" kind of usage.
Rest uses
rte_flow_error_set()as:"
rte_flow_error_set(...);
return -rte_errno;
"Quoted 6 lines
But now it can directly be used as:
"
returnrte_flow_error_set(...);
"What do you think updating to that usage?
Well, that is actually how I originally envisioned this function to be used, but for whatever reason I thought a positive return value was the way to go at the time. Too bad.
Quoted 5 lines
Quoted 1 line
Would you mind updating those drivers as above in a separate patch?
That would be nice but I'm not familiar with most of these PMDs (I'm only updating mlx4 as part of the RSS resurrection series), and there's always a risk of breaking something.
The change looks mechanical, so I believe it won't break anything.
Not directly related to this one, but including this one, recently we kind of have an agreement that library changes doesn't have to update every usage of it, because it is too much work and this is preventing improvements in libraries. (Because sometimes library implementer doesn't know PMD internals to update it properly.)
But currently I have list that PMDs needs to reflect some library work, and list is growing.
Let's assume this work go in without updating PMDs, I believe it will take some time for some PMDs even to recognize this can be changed.
So we either need to change our approach, or find an effective way to pass this information into PMDs. But I believe even passing this information won't be enough, each driver has their own roadmaps, resource allocation, some may tend to not apply these unless PMD is broken.
Any comments?
Quoted 5 lines
The current approach of relying on
rte_errnois OK since this function updates it. I'll put it on my TODO list anyway but given its current size, it's going to be low priority.
Quoted 98 lines · Ferruh Yigit
Quoted 77 lines · Adrien Mazarguil
Quoted 2 lines · Ferruh Yigit
Quoted 1 line · Adrien Mazarguil
On Tue, Oct 10, 2017 at 07:05:30PM +0100, Ferruh Yigit wrote:
<snip>
Quoted 30 lines
Quoted 30 lines
Quoted 4 lines
After above said, API changes one week before integration deadline, a new script and make target for automated header file, I am a little scared :), I will be much relieved to get this in the beginning of the next release cycle.
I can drop the script from this series to speed up inclusion if it there's any concern about it. It's only a helper to update
rte_flow_conv.hafter modifyingrte_flow.h, I thought it could be useful to anyone, hence I've included it but it's pretty much optional.Quoted 1 line
I would like to see more comment on this, specially from PMD maintainers.
Me too. I don't even mind negative ones!
Here's what I plan to do regardless, seeing most concerns so far are with rte_flow_copy()/rte_flow_conv():
- Whether this series is included for 17.11 or later, a v2 is already necessary.
- I will drop the
rte_flow_error()change to submit it instead along another upcoming series for mlx4 where it's the most needed.- We'll then continue to discuss
rte_flow_conv()as a something nice to have but not super urgent to integrate and I'll keep trying to convince everyone it's safe enough.- Once it becomes clear there's no way to have it for 17.11, I'll update this series as a somewhat late deprecation notice for
rte_flow_copy().Quoted 6 lines
Quoted 1 line
Sounds good?
OK. Lets get
rte_flow_error()change and not block mlx4 changes.And I still suggest waiting beginning of the release for rest of the patch. So it can come with optional header auto generation.
OK, I will re-submit an updated version later then.
Quoted 23 lines
Related to the
rte_flow_error_set()modification, as far as I can see it doesn't effect drivers but following drivers are now using it:
drivers/net/bnxt/drivers/net/e1000/drivers/net/enic/drivers/net/i40e/drivers/net/ixgbe/drivers/net/mlx4/drivers/net/mlx5/drivers/net/sfc/drivers/net/tap/There is already tap and mlx4 fixes in the patch to fix "return -rte_flow_error_set(...)" kind of usage.
Rest uses
rte_flow_error_set()as:"
rte_flow_error_set(...);
return -rte_errno;
"Quoted 6 lines
But now it can directly be used as:
"
returnrte_flow_error_set(...);
"What do you think updating to that usage?
Well, that is actually how I originally envisioned this function to be used, but for whatever reason I thought a positive return value was the way to go at the time. Too bad.
Quoted 5 lines
Quoted 1 line
Would you mind updating those drivers as above in a separate patch?
That would be nice but I'm not familiar with most of these PMDs (I'm only updating mlx4 as part of the RSS resurrection series), and there's always a risk of breaking something.
The change looks mechanical, so I believe it won't break anything.
Not directly related to this one, but including this one, recently we kind of have an agreement that library changes doesn't have to update every usage of it, because it is too much work and this is preventing improvements in libraries. (Because sometimes library implementer doesn't know PMD internals to update it properly.)
But currently I have list that PMDs needs to reflect some library work, and list is growing.
Let's assume this work go in without updating PMDs, I believe it will take some time for some PMDs even to recognize this can be changed.
Quoted 6 lines
So we either need to change our approach, or find an effective way to pass this information into PMDs. But I believe even passing this information won't be enough, each driver has their own roadmaps, resource allocation, some may tend to not apply these unless PMD is broken.
Any comments?
I'm generally fine with that, it should likely be considered on a case basis though. Some API changes (e.g. port ID type) also can't avoid modifying all PMDs at once.
It's more or less optional for those that do not break existing APIs, depending on their impact. Say, if some new API involves something to be done on the data path, all PMDs must be validated for performance non-regression. This can't be done properly by people unfamiliar with impacted PMDs (or who may benefit from decreasing performance of competing PMDs :). Either way, if a PMD maintainer disagrees with a change in his/her PMD for any reason, this should not block the API change from entering. The patch for that specific PMD should be ignored in the meantime (possibly to be reworked by one of its maintainers).
Signature · 2 lines
Adrien Mazarguil 6WIND
v2 cover ethdev: add flow API object converter
In reply to Adrien MazarguilThis is a follow up to the "Flow API helpers enhancements" series submitted almost a year ago [1]. The new title is due to the reduced scope of this version.
rte_flow_conv() is a flexible replacement to rte_flow_copy(), itself a temporary solution pending something better [2]. It replaces a lot of duplicated code found in testpmd and removes some of the maintenance burden that developers tend to forget (me included) when modifying pattern item or actions (updating app/test-pmd/config.c to be clear).
This series was unearthed in order to complete the implementation of RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE) in testpmd [3] without having to duplicate existing code once again.
See individual patches for specific changes in this version.
v2 changes:
- rte_flow_copy() is kept, albeit deprecated, no API/ABI impact.
- Updated bonding PMD.
- No more automatic generation of rte_flow_conv.h.
[1] https://mails.dpdk.org/archives/dev/2017-October/077551.html
[2] https://mails.dpdk.org/archives/dev/2017-July/070492.html
[3] Currently the command-line parser (cmdline_flow.c) is aware of these actions, however config.c isn't. Flow rules with such actions cannot be created and cannot be validated with PMDs that implement them.
Adrien Mazarguil (7):
ethdev: add flow API object converter
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function net/failsafe: switch to flow API object conversion function net/bonding: switch to flow API object conversion function
ethdev: deprecate rte_flow_copy function
ethdev: add missing item/actions to flow object converter
app/test-pmd/config.c | 407 +++------------ app/test-pmd/testpmd.h | 7 +- doc/guides/prog_guide/rte_flow.rst | 20 + drivers/net/bonding/rte_eth_bond_api.c | 6 +- drivers/net/bonding/rte_eth_bond_flow.c | 31 +- drivers/net/bonding/rte_eth_bond_private.h | 5 +- drivers/net/failsafe/failsafe_ether.c | 6 +- drivers/net/failsafe/failsafe_flow.c | 31 +- drivers/net/failsafe/failsafe_private.h | 5 +- lib/librte_ethdev/rte_ethdev_version.map | 1 + lib/librte_ethdev/rte_flow.c | 666 ++++++++++++++++++------ lib/librte_ethdev/rte_flow.h | 230 +++++++- 12 files changed, 883 insertions(+), 532 deletions(-)
Signature · 1 line
2.11.0
PATCH v2 1/7 ethdev: add flow API object converter
rte_flow_copy() is bound to duplicate flow rule descriptions (attributes, pattern and list of actions, all at once), however applications sometimes need more flexibility, for instance the ability to duplicate only one of the underlying objects (a single pattern item or action) or retrieve other properties such as their names.
Instead of adding dedicated functions to handle each possible use case, this patch introduces rte_flow_conv(), which supports any number of object conversion operations in an extensible manner.
This patch re-implements rte_flow_copy() as a wrapper to rte_flow_conv().
--
v2 changes:
- Modified patch to keep rte_flow_copy() around instead of removing it entirely. Reworded commit log accordingly.
- Moved failsafe PMD changes to a subsequent patch.
- Re-implemented rte_flow_copy() as a wrapper to rte_flow_conv() to reduce code duplication.
- Tweaked semantics of rte_flow_conv() to return the required number of bytes regardless of the size parameter; a buffer not large enough is not considered to be an error anymore. This change removes the need for a "store" pass in underlying helper functions.
- Renamed and properly documented internal helper functions.
--- doc/guides/prog_guide/rte_flow.rst | 19 + lib/librte_ethdev/rte_ethdev_version.map | 1 + lib/librte_ethdev/rte_flow.c | 553 ++++++++++++++++++-------- lib/librte_ethdev/rte_flow.h | 169 +++++++- 4 files changed, 581 insertions(+), 161 deletions(-)
doc/guides/prog_guide/rte_flow.rst +19 -0
@@ -2419,6 +2419,25 @@ This function initializes ``error`` (if non-NULL) with the provided parameters and sets ``rte_errno`` to ``code``. A negative error ``code`` is then returned. +Object conversion +~~~~~~~~~~~~~~~~~ + +.. code-block:: c + + int + rte_flow_conv(enum rte_flow_conv_op op, + void *dst, + size_t size, + const void *src, + struct rte_flow_error *error); + +Convert ``src`` to ``dst`` according to operation ``op``. Possible +operations include: + +- Attributes, pattern item or action duplication. +- Duplication of an entire pattern or list of actions. +- Duplication of a complete flow rule description. + Caveats -------
lib/librte_ethdev/rte_ethdev_version.map +1 -0
@@ -217,6 +217,7 @@ DPDK_18.08 { global: rte_eth_dev_logtype; + rte_flow_conv; } DPDK_18.05;
lib/librte_ethdev/rte_flow.c +396 -157
624 lines not on this page · raw message
lib/librte_ethdev/rte_flow.h +165 -4
199 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH v2 2/7 ethdev: add flow API item/action name conversion
In reply to Adrien MazarguilThis provides a means for applications to retrieve the name of flow pattern items and actions.
--
v2 changes:
- Replaced rte_flow_conv_name_ptr() with extra is_ptr argument to rte_flow_conv_name() since both functions were almost identical.
- Properly documented internal helper functions.
--- doc/guides/prog_guide/rte_flow.rst | 1 + lib/librte_ethdev/rte_flow.c | 63 +++++++++++++++++++++++++++++++++ lib/librte_ethdev/rte_flow.h | 56 +++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+)
doc/guides/prog_guide/rte_flow.rst +1 -0
@@ -2437,6 +2437,7 @@ operations include: - Attributes, pattern item or action duplication. - Duplication of an entire pattern or list of actions. - Duplication of a complete flow rule description. +- Pattern item or action name retrieval. Caveats -------
lib/librte_ethdev/rte_flow.c +63 -0
81 lines not on this page · raw message
lib/librte_ethdev/rte_flow.h +56 -0
62 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH v2 3/7 app/testpmd: rely on flow API conversion function
In reply to Adrien MazarguilThis commit replaces all local information about pattern items and actions as well as flow rule duplication code with calls to rte_flow_conv().
--- app/test-pmd/config.c | 407 +++++++------------------------------------- app/test-pmd/testpmd.h | 7 +- 2 files changed, 67 insertions(+), 347 deletions(-)
app/test-pmd/config.c +65 -342
@@ -984,324 +984,35 @@ port_mtu_set(portid_t port_id, uint16_t mtu)/* Generic flow management functions. */ -/** Generate flow_item[] entry. */ -#define MK_FLOW_ITEM (t, s) \ - [RTE_FLOW_ITEM_TYPE_ ## t] = { \ - .name = # t, \ - .size = s, \ - } - -/** Information about known flow pattern items. */ -static const struct { -const char *name; -size_t size; -} flow_item[] = { -MK_FLOW_ITEM (END ,0 ), -MK_FLOW_ITEM (VOID ,0 ), -MK_FLOW_ITEM (INVERT ,0 ), -MK_FLOW_ITEM (ANY ,sizeof (struct rte_flow_item_any )), -MK_FLOW_ITEM (PF ,0 ), -MK_FLOW_ITEM (VF ,sizeof (struct rte_flow_item_vf )), -MK_FLOW_ITEM (PHY_PORT ,sizeof (struct rte_flow_item_phy_port )), -MK_FLOW_ITEM (PORT_ID ,sizeof (struct rte_flow_item_port_id )), -MK_FLOW_ITEM (RAW ,sizeof (struct rte_flow_item_raw )), -MK_FLOW_ITEM (ETH ,sizeof (struct rte_flow_item_eth )), -MK_FLOW_ITEM (VLAN ,sizeof (struct rte_flow_item_vlan )), -MK_FLOW_ITEM (IPV4 ,sizeof (struct rte_flow_item_ipv4 )), -MK_FLOW_ITEM (IPV6 ,sizeof (struct rte_flow_item_ipv6 )), -MK_FLOW_ITEM (ICMP ,sizeof (struct rte_flow_item_icmp )), -MK_FLOW_ITEM (UDP ,sizeof (struct rte_flow_item_udp )), -MK_FLOW_ITEM (TCP ,sizeof (struct rte_flow_item_tcp )), -MK_FLOW_ITEM (SCTP ,sizeof (struct rte_flow_item_sctp )), -MK_FLOW_ITEM (VXLAN ,sizeof (struct rte_flow_item_vxlan )), -MK_FLOW_ITEM (E_TAG ,sizeof (struct rte_flow_item_e_tag )), -MK_FLOW_ITEM (NVGRE ,sizeof (struct rte_flow_item_nvgre )), -MK_FLOW_ITEM (MPLS ,sizeof (struct rte_flow_item_mpls )), -MK_FLOW_ITEM (GRE ,sizeof (struct rte_flow_item_gre )), -MK_FLOW_ITEM (FUZZY ,sizeof (struct rte_flow_item_fuzzy )), -MK_FLOW_ITEM (GTP ,sizeof (struct rte_flow_item_gtp )), -MK_FLOW_ITEM (GTPC ,sizeof (struct rte_flow_item_gtp )), -MK_FLOW_ITEM (GTPU ,sizeof (struct rte_flow_item_gtp )), -MK_FLOW_ITEM (GENEVE ,sizeof (struct rte_flow_item_geneve )), -MK_FLOW_ITEM (VXLAN_GPE ,sizeof (struct rte_flow_item_vxlan_gpe )), -MK_FLOW_ITEM (ARP_ETH_IPV4 ,sizeof (struct rte_flow_item_arp_eth_ipv4 )), -MK_FLOW_ITEM (IPV6_EXT ,sizeof (struct rte_flow_item_ipv6_ext )), -MK_FLOW_ITEM (ICMP6 ,sizeof (struct rte_flow_item_icmp6 )), -MK_FLOW_ITEM (ICMP6_ND_NS ,sizeof (struct rte_flow_item_icmp6_nd_ns )), -MK_FLOW_ITEM (ICMP6_ND_NA ,sizeof (struct rte_flow_item_icmp6_nd_na )), -MK_FLOW_ITEM (ICMP6_ND_OPT ,sizeof (struct rte_flow_item_icmp6_nd_opt )), -MK_FLOW_ITEM (ICMP6_ND_OPT_SLA_ETH , -sizeof (struct rte_flow_item_icmp6_nd_opt_sla_eth )), -MK_FLOW_ITEM (ICMP6_ND_OPT_TLA_ETH , -sizeof (struct rte_flow_item_icmp6_nd_opt_tla_eth )), -}; - -/** Pattern item specification types. */ -enum item_spec_type { -ITEM_SPEC , -ITEM_LAST , -ITEM_MASK , -}; - -/** Compute storage space needed by item specification and copy it. */ -static size_t -flow_item_spec_copy (void *buf,const struct rte_flow_item *item, -enum item_spec_type type) -{ -size_t size =0 ; -const void *data = - type ==ITEM_SPEC ? item->spec : - type ==ITEM_LAST ? item->last : - type ==ITEM_MASK ? item->mask : -NULL ; - -if (!item->spec || !data) - goto empty; -switch (item->type) { -union { -const struct rte_flow_item_raw *raw; - } spec; -union { -const struct rte_flow_item_raw *raw; - } last; -union { -const struct rte_flow_item_raw *raw; - } mask; -union { -const struct rte_flow_item_raw *raw; - } src; -union { -struct rte_flow_item_raw *raw; - } dst; -size_t off; - -case RTE_FLOW_ITEM_TYPE_RAW : - spec.raw = item->spec; - last.raw = item->last ? item->last : item->spec; - mask.raw = item->mask ? item->mask : &rte_flow_item_raw_mask; - src.raw = data; - dst.raw = buf; - off =RTE_ALIGN_CEIL (sizeof (struct rte_flow_item_raw ), -sizeof (*src.raw->pattern)); -if (type ==ITEM_SPEC || - (type ==ITEM_MASK && - ((spec.raw->length & mask.raw->length) >= - (last.raw->length & mask.raw->length)))) - size = spec.raw->length & mask.raw->length; -else - size = last.raw->length & mask.raw->length; - size = off + size *sizeof (*src.raw->pattern); -if (dst.raw) { -memcpy (dst.raw, src.raw,sizeof (*src.raw)); - dst.raw->pattern =memcpy ((uint8_t *)dst.raw + off, - src.raw->pattern, - size - off); - } -break ; -default : - size = flow_item[item->type].size; -if (buf) -memcpy (buf, data, size); -break ; - } -empty: -return RTE_ALIGN_CEIL (size,sizeof (double )); -} - -/** Generate flow_action[] entry. */ -#define MK_FLOW_ACTION (t, s) \ - [RTE_FLOW_ACTION_TYPE_ ## t] = { \ - .name = # t, \ - .size = s, \ - } - -/** Information about known flow actions. */ -static const struct { -const char *name; -size_t size; -} flow_action[] = { -MK_FLOW_ACTION (END ,0 ), -MK_FLOW_ACTION (VOID ,0 ), -MK_FLOW_ACTION (PASSTHRU ,0 ), -MK_FLOW_ACTION (MARK ,sizeof (struct rte_flow_action_mark )), -MK_FLOW_ACTION (FLAG ,0 ), -MK_FLOW_ACTION (QUEUE ,sizeof (struct rte_flow_action_queue )), -MK_FLOW_ACTION (DROP ,0 ), -MK_FLOW_ACTION (COUNT ,sizeof (struct rte_flow_action_count )), -MK_FLOW_ACTION (RSS ,sizeof (struct rte_flow_action_rss )), -MK_FLOW_ACTION (PF ,0 ), -MK_FLOW_ACTION (VF ,sizeof (struct rte_flow_action_vf )), -MK_FLOW_ACTION (PHY_PORT ,sizeof (struct rte_flow_action_phy_port )), -MK_FLOW_ACTION (PORT_ID ,sizeof (struct rte_flow_action_port_id )), -MK_FLOW_ACTION (METER ,sizeof (struct rte_flow_action_meter )), -MK_FLOW_ACTION (OF_SET_MPLS_TTL , -sizeof (struct rte_flow_action_of_set_mpls_ttl )), -MK_FLOW_ACTION (OF_DEC_MPLS_TTL ,0 ), -MK_FLOW_ACTION (OF_SET_NW_TTL , -sizeof (struct rte_flow_action_of_set_nw_ttl )), -MK_FLOW_ACTION (OF_DEC_NW_TTL ,0 ), -MK_FLOW_ACTION (OF_COPY_TTL_OUT ,0 ), -MK_FLOW_ACTION (OF_COPY_TTL_IN ,0 ), -MK_FLOW_ACTION (OF_POP_VLAN ,0 ), -MK_FLOW_ACTION (OF_PUSH_VLAN , -sizeof (struct rte_flow_action_of_push_vlan )), -MK_FLOW_ACTION (OF_SET_VLAN_VID , -sizeof (struct rte_flow_action_of_set_vlan_vid )), -MK_FLOW_ACTION (OF_SET_VLAN_PCP , -sizeof (struct rte_flow_action_of_set_vlan_pcp )), -MK_FLOW_ACTION (OF_POP_MPLS , -sizeof (struct rte_flow_action_of_pop_mpls )), -MK_FLOW_ACTION (OF_PUSH_MPLS , -sizeof (struct rte_flow_action_of_push_mpls )), -}; - -/** Compute storage space needed by action configuration and copy it. */ -static size_t -flow_action_conf_copy (void *buf,const struct rte_flow_action *action) -{ -size_t size =0 ; - -if (!action->conf) - goto empty; -switch (action->type) { -union { -const struct rte_flow_action_rss *rss; - } src; -union { -struct rte_flow_action_rss *rss; - } dst; -size_t off; - -case RTE_FLOW_ACTION_TYPE_RSS : - src.rss = action->conf; - dst.rss = buf; - off =0 ; -if (dst.rss) - *dst.rss = (struct rte_flow_action_rss ){ - .func = src.rss->func, - .level = src.rss->level, - .types = src.rss->types, - .key_len = src.rss->key_len, - .queue_num = src.rss->queue_num, - }; - off +=sizeof (*src.rss); -if (src.rss->key_len) { - off =RTE_ALIGN_CEIL (off,sizeof (double )); - size =sizeof (*src.rss->key) * src.rss->key_len; -if (dst.rss) - dst.rss->key =memcpy - ((void *)((uintptr_t )dst.rss + off), - src.rss->key, size); - off += size; - } -if (src.rss->queue_num) { - off =RTE_ALIGN_CEIL (off,sizeof (double )); - size =sizeof (*src.rss->queue) * src.rss->queue_num; -if (dst.rss) - dst.rss->queue =memcpy - ((void *)((uintptr_t )dst.rss + off), - src.rss->queue, size); - off += size; - } - size = off; -break ; -default : - size = flow_action[action->type].size; -if (buf) -memcpy (buf, action->conf, size); -break ; - } -empty: -return RTE_ALIGN_CEIL (size,sizeof (double )); -} -/** Generate a port_flow entry from attributes/pattern/actions. */ static struct port_flow *port_flow_new (const struct rte_flow_attr *attr,const struct rte_flow_item *pattern, -const struct rte_flow_action *actions) -{ -const struct rte_flow_item *item; -const struct rte_flow_action *action; -struct port_flow *pf =NULL ; -size_t tmp; -size_t off1 =0 ; -size_t off2 =0 ; -int err =ENOTSUP ; - -store: - item = pattern; -if (pf) - pf->pattern = (void *)&pf->data[off1]; -do { -struct rte_flow_item *dst =NULL ; - -if ((unsigned int )item->type >=RTE_DIM (flow_item) || - !flow_item[item->type].name) - goto notsup; -if (pf) - dst =memcpy (pf->data + off1, item,sizeof (*item)); - off1 +=sizeof (*item); -if (item->spec) { -if (pf) - dst->spec = pf->data + off2; - off2 +=flow_item_spec_copy - (pf ? pf->data + off2 :NULL , item,ITEM_SPEC ); - } -if (item->last) { -if (pf) - dst->last = pf->data + off2; - off2 +=flow_item_spec_copy - (pf ? pf->data + off2 :NULL , item,ITEM_LAST ); - } -if (item->mask) { -if (pf) - dst->mask = pf->data + off2; - off2 +=flow_item_spec_copy - (pf ? pf->data + off2 :NULL , item,ITEM_MASK ); - } - off2 =RTE_ALIGN_CEIL (off2,sizeof (double )); - }while ((item++)->type !=RTE_FLOW_ITEM_TYPE_END ); - off1 =RTE_ALIGN_CEIL (off1,sizeof (double )); - action = actions; -if (pf) - pf->actions = (void *)&pf->data[off1]; -do { -struct rte_flow_action *dst =NULL ; - -if ((unsigned int )action->type >=RTE_DIM (flow_action) || - !flow_action[action->type].name) - goto notsup; -if (pf) - dst =memcpy (pf->data + off1, action,sizeof (*action)); - off1 +=sizeof (*action); -if (action->conf) { -if (pf) - dst->conf = pf->data + off2; - off2 +=flow_action_conf_copy - (pf ? pf->data + off2 :NULL , action); - } - off2 =RTE_ALIGN_CEIL (off2,sizeof (double )); - }while ((action++)->type !=RTE_FLOW_ACTION_TYPE_END ); -if (pf !=NULL ) +const struct rte_flow_action *actions, +struct rte_flow_error *error) +{ +const struct rte_flow_conv_rule rule = { + .attr_ro = attr, + .pattern_ro = pattern, + .actions_ro = actions, + }; +struct port_flow *pf; +int ret; + + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE ,NULL ,0 , &rule, error); +if (ret <0 ) +return NULL ; + pf =calloc (1 , offsetof(struct port_flow , rule) + ret); +if (!pf) { +rte_flow_error_set + (error, errno,RTE_FLOW_ERROR_TYPE_UNSPECIFIED ,NULL , +"calloc() failed" ); +return NULL ; + } +if (rte_flow_conv (RTE_FLOW_CONV_OP_RULE , &pf->rule, ret, &rule, + error) >=0 )return pf; - off1 =RTE_ALIGN_CEIL (off1,sizeof (double )); - tmp =RTE_ALIGN_CEIL (offsetof(struct port_flow , data),sizeof (double )); - pf =calloc (1 , tmp + off1 + off2); -if (pf ==NULL ) - err = errno; -else { - *pf = (const struct port_flow ){ - .size = tmp + off1 + off2, - .attr = *attr, - }; - tmp -= offsetof(struct port_flow , data); - off2 = tmp + off1; - off1 = tmp; - goto store; - } -notsup: - rte_errno = err; +free (pf);return NULL ; }
@@ -1391,13 +1102,10 @@ port_flow_create(portid_t port_id, id = port->flow_list->id +1 ; }else id =0 ; - pf =port_flow_new (attr, pattern, actions); + pf =port_flow_new (attr, pattern, actions, &error);if (!pf) { -int err = rte_errno; - -printf ("Cannot allocate flow: %s\n" ,rte_strerror (err));rte_flow_destroy (port_id, flow,NULL ); -return -err; +return port_flow_complain (&error); } pf->next = port->flow_list; pf->id = id;
@@ -1489,6 +1197,7 @@ port_flow_query(portid_t port_id, uint32_t rule,union {struct rte_flow_query_count count; } query; +int ret;if (port_id_is_invalid (port_id,ENABLED_WARN ) || port_id == (portid_t )RTE_PORT_ALL )
@@ -1501,11 +1210,10 @@ port_flow_query(portid_t port_id, uint32_t rule,printf ("Flow rule #%u not found\n" , rule);return -ENOENT ; } -if ((unsigned int )action->type >=RTE_DIM (flow_action) || - !flow_action[action->type].name) - name ="unknown" ; -else - name = flow_action[action->type].name; + ret =rte_flow_conv (RTE_FLOW_CONV_OP_ACTION_NAME_PTR , + &name,sizeof (name), action, &error); +if (ret <0 ) +return port_flow_complain (&error);switch (action->type) {case RTE_FLOW_ACTION_TYPE_COUNT :break ;
@@ -1558,48 +1266,63 @@ port_flow_list(portid_t port_id, uint32_t n, const uint32_t group[n])/* Sort flows by group, priority and ID. */ for (pf = port->flow_list; pf !=NULL ; pf = pf->next) {struct port_flow **tmp; +const struct rte_flow_attr *curr = pf->rule.attr;if (n) {/* Filter out unwanted groups. */ for (i =0 ; i != n; ++i) -if (pf->attr.group == group[i]) +if (curr->group == group[i])break ;if (i == n)continue ; } - tmp = &list; -while (*tmp && - (pf->attr.group > (*tmp)->attr.group || - (pf->attr.group == (*tmp)->attr.group && - pf->attr.priority > (*tmp)->attr.priority) || - (pf->attr.group == (*tmp)->attr.group && - pf->attr.priority == (*tmp)->attr.priority && - pf->id > (*tmp)->id))) - tmp = &(*tmp)->tmp; +for (tmp = &list; *tmp; tmp = &(*tmp)->tmp) { +const struct rte_flow_attr *comp = (*tmp)->rule.attr; + +if (curr->group > comp->group || + (curr->group == comp->group && + curr->priority > comp->priority) || + (curr->group == comp->group && + curr->priority == comp->priority && + pf->id > (*tmp)->id)) +continue ; +break ; + } pf->tmp = *tmp; *tmp = pf; }printf ("ID\tGroup\tPrio\tAttr\tRule\n" );for (pf = list; pf !=NULL ; pf = pf->tmp) { -const struct rte_flow_item *item = pf->pattern; -const struct rte_flow_action *action = pf->actions; +const struct rte_flow_item *item = pf->rule.pattern; +const struct rte_flow_action *action = pf->rule.actions; +const char *name;printf ("%" PRIu32"\t%" PRIu32"\t%" PRIu32"\t%c%c%c\t" , pf->id, - pf->attr.group, - pf->attr.priority, - pf->attr.ingress ?'i' :'-' , - pf->attr.egress ?'e' :'-' , - pf->attr.transfer ?'t' :'-' ); + pf->rule.attr->group, + pf->rule.attr->priority, + pf->rule.attr->ingress ?'i' :'-' , + pf->rule.attr->egress ?'e' :'-' , + pf->rule.attr->transfer ?'t' :'-' );while (item->type !=RTE_FLOW_ITEM_TYPE_END ) { +if (rte_flow_conv (RTE_FLOW_CONV_OP_ITEM_NAME_PTR , + &name,sizeof (name), + (void *)(uintptr_t )item->type, +NULL ) <=0 ) + name ="[UNKNOWN]" ;if (item->type !=RTE_FLOW_ITEM_TYPE_VOID ) -printf ("%s " , flow_item[item->type].name); +printf ("%s " , name); ++item; }printf ("=>" );while (action->type !=RTE_FLOW_ACTION_TYPE_END ) { +if (rte_flow_conv (RTE_FLOW_CONV_OP_ACTION_NAME_PTR , + &name,sizeof (name), + (void *)(uintptr_t )action->type, +NULL ) <=0 ) + name ="[UNKNOWN]" ;if (action->type !=RTE_FLOW_ACTION_TYPE_VOID ) -printf (" %s" , flow_action[action->type].name); +printf (" %s" , name); ++action; }printf ("\n" );
app/test-pmd/testpmd.h +2 -5
@@ -124,15 +124,12 @@ struct fwd_stream {/** Descriptor for a single flow. */ struct port_flow { -size_t size;/**< Allocated space including data[]. */ struct port_flow *next;/**< Next flow in list. */ struct port_flow *tmp;/**< Temporary linking. */ uint32_t id;/**< Flow rule ID. */ struct rte_flow *flow;/**< Opaque flow object returned by PMD. */ -struct rte_flow_attr attr;/**< Attributes. */ -struct rte_flow_item *pattern;/**< Pattern. */ -struct rte_flow_action *actions;/**< Actions. */ -uint8_t data[];/**< Storage for pattern/actions. */ +struct rte_flow_conv_rule rule;/* Saved flow rule description. */ +uint8_t data[];/**< Storage for flow rule description */ };#ifdef SOFTNIC
Signature · 1 line
2.11.0
PATCH v2 4/7 net/failsafe: switch to flow API object conversion function
In reply to Adrien MazarguilThis patch replaces rte_flow_copy() with rte_flow_conv().
--
v2 changes:
- Patch was split from "ethdev: add flow API object converter".
--- drivers/net/failsafe/failsafe_ether.c | 6 +++--- drivers/net/failsafe/failsafe_flow.c | 31 +++++++++++++++++++++------- drivers/net/failsafe/failsafe_private.h | 5 ++++- 3 files changed, 31 insertions(+), 11 deletions(-)
drivers/net/failsafe/failsafe_ether.c +3 -3
@@ -230,9 +230,9 @@ fs_eth_dev_conf_apply(struct rte_eth_dev *dev,DEBUG ("Creating flow #%" PRIu32, i++); flow->flows[SUB_ID (sdev)] =rte_flow_create (PORT_ID (sdev), - &flow->fd->attr, - flow->fd->items, - flow->fd->actions, + flow->rule.attr, + flow->rule.pattern, + flow->rule.actions, &ferror); ret = rte_errno;if (ret)
drivers/net/failsafe/failsafe_flow.c +24 -7
@@ -3,8 +3,11 @@* Copyright 2017 Mellanox Technologies, Ltd */ +#include <stddef.h> +#include <string.h> #include <sys/queue.h> +#include <rte_errno.h> #include <rte_malloc.h> #include <rte_tailq.h> #include <rte_flow.h>
@@ -18,19 +21,33 @@ fs_flow_allocate(const struct rte_flow_attr *attr, const struct rte_flow_action *actions) {struct rte_flow *flow; -size_t fdsz; +const struct rte_flow_conv_rule rule = { + .attr_ro = attr, + .pattern_ro = items, + .actions_ro = actions, + }; +struct rte_flow_error error; +int ret; - fdsz =rte_flow_copy (NULL ,0 , attr, items, actions); - flow =rte_zmalloc (NULL , -sizeof (struct rte_flow ) + fdsz, + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE ,NULL ,0 , &rule, &error); +if (ret <0 ) { +ERROR ("Unable to process flow rule (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno)); +return NULL ; + } + flow =rte_zmalloc (NULL , offsetof(struct rte_flow , rule) + ret,RTE_CACHE_LINE_SIZE );if (flow ==NULL ) {ERROR ("Could not allocate new flow" );return NULL ; } - flow->fd = (void *)((uintptr_t )flow +sizeof (*flow)); -if (rte_flow_copy (flow->fd, fdsz, attr, items, actions) != fdsz) { -ERROR ("Failed to copy flow description" ); + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE , &flow->rule, ret, &rule, + &error); +if (ret <0 ) { +ERROR ("Failed to copy flow rule (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno));rte_free (flow);return NULL ; }
drivers/net/failsafe/failsafe_private.h +4 -1
@@ -6,6 +6,7 @@#ifndef _RTE_ETH_FAILSAFE_PRIVATE_H_#define _RTE_ETH_FAILSAFE_PRIVATE_H_ +#include <stdint.h> #include <sys/queue.h> #include <pthread.h>
@@ -13,6 +14,7 @@#include <rte_dev.h> #include <rte_ethdev_driver.h> #include <rte_devargs.h> +#include <rte_flow.h> #include <rte_interrupts.h> #define FAILSAFE_DRIVER_NAME "Fail-safe PMD"
@@ -81,7 +83,8 @@ struct rte_flow {/* sub_flows */ struct rte_flow *flows[FAILSAFE_MAX_ETHPORTS ];/* flow description for synchronization */ -struct rte_flow_desc *fd; +struct rte_flow_conv_rule rule; +uint8_t rule_data[]; };enum dev_state {
Signature · 1 line
2.11.0
PATCH v2 5/7 net/bonding: switch to flow API object conversion function
In reply to Adrien MazarguilThis patch replaces rte_flow_copy() with rte_flow_conv().
--
v2 changes:
- Patch was not present in original series.
--- drivers/net/bonding/rte_eth_bond_api.c | 6 ++--- drivers/net/bonding/rte_eth_bond_flow.c | 31 +++++++++++++++++++------ drivers/net/bonding/rte_eth_bond_private.h | 5 +++- 3 files changed, 31 insertions(+), 11 deletions(-)
drivers/net/bonding/rte_eth_bond_api.c +3 -3
@@ -245,9 +245,9 @@ slave_rte_flow_prepare(uint16_t slave_id, struct bond_dev_private *internals) } TAILQ_FOREACH(flow, &internals->flow_list, next) { flow->flows[slave_id] =rte_flow_create (slave_port_id, - &flow->fd->attr, - flow->fd->items, - flow->fd->actions, + flow->rule.attr, + flow->rule.pattern, + flow->rule.actions, &ferror);if (flow->flows[slave_id] ==NULL ) { RTE_BOND_LOG(ERR, "Cannot create flow for slave"
drivers/net/bonding/rte_eth_bond_flow.c +24 -7
@@ -2,8 +2,11 @@* Copyright 2018 Mellanox Technologies, Ltd */ +#include <stddef.h> +#include <string.h> #include <sys/queue.h> +#include <rte_errno.h> #include <rte_malloc.h> #include <rte_tailq.h> #include <rte_flow.h>
@@ -16,19 +19,33 @@ bond_flow_alloc(int numa_node, const struct rte_flow_attr *attr, const struct rte_flow_action *actions) {struct rte_flow *flow; -size_t fdsz; +const struct rte_flow_conv_rule rule = { + .attr_ro = attr, + .pattern_ro = items, + .actions_ro = actions, + }; +struct rte_flow_error error; +int ret; - fdsz =rte_flow_copy (NULL ,0 , attr, items, actions); - flow =rte_zmalloc_socket (NULL ,sizeof (struct rte_flow ) + fdsz, + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE ,NULL ,0 , &rule, &error); +if (ret <0 ) { +RTE_BOND_LOG (ERR ,"Unable to process flow rule (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno)); +return NULL ; + } + flow =rte_zmalloc_socket (NULL , offsetof(struct rte_flow , rule) + ret,RTE_CACHE_LINE_SIZE , numa_node);if (unlikely (flow ==NULL )) {RTE_BOND_LOG (ERR ,"Could not allocate new flow" );return NULL ; } - flow->fd = (void *)((uintptr_t )flow +sizeof (*flow)); -if (unlikely (rte_flow_copy (flow->fd, fdsz, attr, items, actions) != - fdsz)) { -RTE_BOND_LOG (ERR ,"Failed to copy flow description" ); + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE , &flow->rule, ret, &rule, + &error); +if (ret <0 ) { +RTE_BOND_LOG (ERR ,"Failed to copy flow rule (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno));rte_free (flow);return NULL ; }
drivers/net/bonding/rte_eth_bond_private.h +4 -1
@@ -5,9 +5,11 @@#ifndef _RTE_ETH_BOND_PRIVATE_H_#define _RTE_ETH_BOND_PRIVATE_H_ +#include <stdint.h> #include <sys/queue.h> #include <rte_ethdev_driver.h> +#include <rte_flow.h> #include <rte_spinlock.h> #include <rte_bitmap.h> #include <rte_flow_driver.h>
@@ -93,7 +95,8 @@ struct rte_flow {/* Slaves flows */ struct rte_flow *flows[RTE_MAX_ETHPORTS ];/* Flow description for synchronization */ -struct rte_flow_desc *fd; +struct rte_flow_conv_rule rule; +uint8_t rule_data[]; }; typedef void (*burst_xmit_hash_t)(struct rte_mbuf **buf, uint16_t nb_pkts,
Signature · 1 line
2.11.0
PATCH v2 6/7 ethdev: deprecate rte_flow_copy function
In reply to Adrien MazarguilNo users left for this function, time to deprecate it.
--
v2 changes:
- Patch was not present in original series.
--- lib/librte_ethdev/rte_flow.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
lib/librte_ethdev/rte_flow.h +6 -1
@@ -2332,6 +2332,7 @@ rte_flow_error_set(struct rte_flow_error *error, const char *message);/** +* @deprecated * @see rte_flow_copy() */ struct rte_flow_desc {
@@ -2343,10 +2344,13 @@ struct rte_flow_desc { };/** +* @deprecated * Copy an rte_flow rule description. * * This interface is kept for compatibility with older applications but is -* implemented as a wrapper to rte_flow_conv(). +* implemented as a wrapper to rte_flow_conv(). It is deprecated due to its +* lack of flexibility and reliance on a type unusable with C++ programs +* (struct rte_flow_desc). * * @param[in] fd * Flow rule description.
@@ -2365,6 +2369,7 @@ struct rte_flow_desc {* If len is lower than the size of the flow, the number of bytes that would * have been written to desc had it been sufficient. Nothing is written. */ +__rte_deprecated size_t rte_flow_copy(struct rte_flow_desc *fd, size_t len, const struct rte_flow_attr *attr,
Signature · 1 line
2.11.0
PATCH v2 7/7 ethdev: add missing item/actions to flow object converter
In reply to Adrien MazarguilSeveral pattern items and actions were never handled by rte_flow_copy() because their descriptions were missing. rte_flow_conv() inherited this deficiency.
This patch adds them and reorders others to match rte_flow.h. It doesn't pose as a fix because so far no one has complained about it and rte_flow_conv() would have to be backported as well: this function is the only sane approach to handle VXLAN and NVGRE encap definitions.
As a matter of fact, it's the last missing piece to finally allow testpmd users to request the creation of VXLAN/NVGRE encap/decap flow rules without getting rejected outright.
--
v2 changes:
- Patch was not present in original series.
--- lib/librte_ethdev/rte_flow.c | 50 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)
lib/librte_ethdev/rte_flow.c +48 -2
@@ -51,10 +51,15 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {MK_FLOW_ITEM (TCP ,sizeof (struct rte_flow_item_tcp )),MK_FLOW_ITEM (SCTP ,sizeof (struct rte_flow_item_sctp )),MK_FLOW_ITEM (VXLAN ,sizeof (struct rte_flow_item_vxlan )), -MK_FLOW_ITEM (MPLS ,sizeof (struct rte_flow_item_mpls )), -MK_FLOW_ITEM (GRE ,sizeof (struct rte_flow_item_gre )),MK_FLOW_ITEM (E_TAG ,sizeof (struct rte_flow_item_e_tag )),MK_FLOW_ITEM (NVGRE ,sizeof (struct rte_flow_item_nvgre )), +MK_FLOW_ITEM (MPLS ,sizeof (struct rte_flow_item_mpls )), +MK_FLOW_ITEM (GRE ,sizeof (struct rte_flow_item_gre )), +MK_FLOW_ITEM (FUZZY ,sizeof (struct rte_flow_item_fuzzy )), +MK_FLOW_ITEM (GTP ,sizeof (struct rte_flow_item_gtp )), +MK_FLOW_ITEM (GTPC ,sizeof (struct rte_flow_item_gtp )), +MK_FLOW_ITEM (GTPU ,sizeof (struct rte_flow_item_gtp )), +MK_FLOW_ITEM (ESP ,sizeof (struct rte_flow_item_esp )),MK_FLOW_ITEM (GENEVE ,sizeof (struct rte_flow_item_geneve )),MK_FLOW_ITEM (VXLAN_GPE ,sizeof (struct rte_flow_item_vxlan_gpe )), MK_FLOW_ITEM(ARP_ETH_IPV4, sizeof(struct rte_flow_item_arp_eth_ipv4)),
@@ -67,6 +72,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = { sizeof(struct rte_flow_item_icmp6_nd_opt_sla_eth)),MK_FLOW_ITEM (ICMP6_ND_OPT_TLA_ETH ,sizeof (struct rte_flow_item_icmp6_nd_opt_tla_eth )), + MK_FLOW_ITEM(MARK, sizeof(struct rte_flow_item_mark)), };/** Generate flow_action[] entry. */
@@ -81,6 +87,7 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = {MK_FLOW_ACTION (END ,0 ),MK_FLOW_ACTION (VOID ,0 ),MK_FLOW_ACTION (PASSTHRU ,0 ), +MK_FLOW_ACTION (JUMP ,sizeof (struct rte_flow_action_jump )),MK_FLOW_ACTION (MARK ,sizeof (struct rte_flow_action_mark )),MK_FLOW_ACTION (FLAG ,0 ), MK_FLOW_ACTION(QUEUE, sizeof(struct rte_flow_action_queue)),
@@ -91,6 +98,8 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = {MK_FLOW_ACTION (VF ,sizeof (struct rte_flow_action_vf )),MK_FLOW_ACTION (PHY_PORT ,sizeof (struct rte_flow_action_phy_port )),MK_FLOW_ACTION (PORT_ID ,sizeof (struct rte_flow_action_port_id )), +MK_FLOW_ACTION (METER ,sizeof (struct rte_flow_action_meter )), +MK_FLOW_ACTION (SECURITY ,sizeof (struct rte_flow_action_security )),MK_FLOW_ACTION (OF_SET_MPLS_TTL ,sizeof (struct rte_flow_action_of_set_mpls_ttl )), MK_FLOW_ACTION(OF_DEC_MPLS_TTL, 0),
@@ -110,6 +119,10 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = { sizeof(struct rte_flow_action_of_pop_mpls)),MK_FLOW_ACTION (OF_PUSH_MPLS ,sizeof (struct rte_flow_action_of_push_mpls )), +MK_FLOW_ACTION (VXLAN_ENCAP ,sizeof (struct rte_flow_action_vxlan_encap )), +MK_FLOW_ACTION (VXLAN_DECAP ,0 ), +MK_FLOW_ACTION (NVGRE_ENCAP ,sizeof (struct rte_flow_action_vxlan_encap )), + MK_FLOW_ACTION(NVGRE_DECAP, 0), }; static int
@@ -407,11 +420,16 @@ rte_flow_conv_action_conf(void *buf, const size_t size,switch (action->type) {union {const struct rte_flow_action_rss *rss; +const struct rte_flow_action_vxlan_encap *vxlan_encap; +const struct rte_flow_action_nvgre_encap *nvgre_encap; } src;union {struct rte_flow_action_rss *rss; +struct rte_flow_action_vxlan_encap *vxlan_encap; +struct rte_flow_action_nvgre_encap *nvgre_encap; } dst;size_t tmp; +int ret;case RTE_FLOW_ACTION_TYPE_RSS : src.rss = action->conf;
@@ -445,6 +463,34 @@ rte_flow_conv_action_conf(void *buf, const size_t size, off += tmp; }break ; +case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP : +case RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP : + src.vxlan_encap = action->conf; + dst.vxlan_encap = buf; +RTE_BUILD_BUG_ON (sizeof (*src.vxlan_encap) != +sizeof (*src.nvgre_encap) || + offsetof(struct rte_flow_action_vxlan_encap , + definition) != + offsetof(struct rte_flow_action_nvgre_encap , + definition)); + off =sizeof (*dst.vxlan_encap); +if (src.vxlan_encap->definition) { + off =RTE_ALIGN_CEIL + (off,sizeof (*dst.vxlan_encap->definition)); + ret =rte_flow_conv + (RTE_FLOW_CONV_OP_PATTERN , + (void *)((uintptr_t )dst.vxlan_encap + off), + size > off ? size - off :0 , + src.vxlan_encap->definition,NULL ); +if (ret <0 ) +return 0 ; +if (size >= off + ret) + dst.vxlan_encap->definition = + (void *)((uintptr_t )dst.vxlan_encap + + off); + off += ret; + } +break ;default : off = rte_flow_desc_action[action->type].size;rte_memcpy (buf, action->conf, (size > off ? off : size));
Signature · 1 line
2.11.0
Re: [PATCH v2 0/7] ethdev: add flow API object converter
In reply to Adrien Mazarguil03/08/2018 15:36, Adrien Mazarguil:
Quoted 8 lines
Adrien Mazarguil (7):
ethdev: add flow API object converter
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function net/failsafe: switch to flow API object conversion function net/bonding: switch to flow API object conversion function
ethdev: deprecaterte_flow_copyfunction
ethdev: add missing item/actions to flow object converter
This series will be considered for 18.11.
As you plan to deprecate rte_flow_copy function, please send a deprecation notice which could enter in 18.08 release notes.
Thanks
Re: [PATCH v2 0/7] ethdev: add flow API object converter
In reply to Adrien MazarguilQuoted 28 lines · Adrien Mazarguil
This is a follow up to the "Flow API helpers enhancements" series submitted almost a year ago [1]. The new title is due to the reduced scope of this version.
rte_flow_conv()is a flexible replacement torte_flow_copy(), itself a temporary solution pending something better [2]. It replaces a lot of duplicated code found in testpmd and removes some of the maintenance burden that developers tend to forget (me included) when modifying pattern item or actions (updatingapp/test-pmd/config.cto be clear).This series was unearthed in order to complete the implementation of
RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE)in testpmd [3] without having to duplicate existing code once again.See individual patches for specific changes in this version.
v2 changes:
-
rte_flow_copy()is kept, albeit deprecated, no API/ABI impact.
- Updated bonding PMD.
- No more automatic generation ofrte_flow_conv.h.[1] https://mails.dpdk.org/archives/dev/2017-October/077551.html
[2] https://mails.dpdk.org/archives/dev/2017-July/070492.html
[3] Currently the command-line parser (cmdline_flow.c) is aware of these actions, howeverconfig.cisn't. Flow rules with such actions cannot be created and cannot be validated with PMDs that implement them.
Quoted 8 lines
Adrien Mazarguil (7):
ethdev: add flow API object converter
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function net/failsafe: switch to flow API object conversion function net/bonding: switch to flow API object conversion function
ethdev: deprecaterte_flow_copyfunction
ethdev: add missing item/actions to flow object converter
Patch needs to be rebased to target v18.11 (in map file), and indeed new APIs (rte_flow_conv) needs to be experimental.
And needs to remove deprecation notice in this patchset.
Also do you think does make sense to announce this change in release notes?
Apart from above, any volunteer for reviewing actual implementation?
Re: [PATCH v2 0/7] ethdev: add flow API object converter
Quoted 38 lines · Ferruh Yigit
Quoted 28 lines · Adrien Mazarguil
This is a follow up to the "Flow API helpers enhancements" series submitted almost a year ago [1]. The new title is due to the reduced scope of this version.
rte_flow_conv()is a flexible replacement torte_flow_copy(), itself a temporary solution pending something better [2]. It replaces a lot of duplicated code found in testpmd and removes some of the maintenance burden that developers tend to forget (me included) when modifying pattern item or actions (updatingapp/test-pmd/config.cto be clear).This series was unearthed in order to complete the implementation of
RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE)in testpmd [3] without having to duplicate existing code once again.See individual patches for specific changes in this version.
v2 changes:
-
rte_flow_copy()is kept, albeit deprecated, no API/ABI impact.
- Updated bonding PMD.
- No more automatic generation ofrte_flow_conv.h.[1] https://mails.dpdk.org/archives/dev/2017-October/077551.html
[2] https://mails.dpdk.org/archives/dev/2017-July/070492.html
[3] Currently the command-line parser (cmdline_flow.c) is aware of these actions, howeverconfig.cisn't. Flow rules with such actions cannot be created and cannot be validated with PMDs that implement them.Quoted 8 lines
Adrien Mazarguil (7):
ethdev: add flow API object converter
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function net/failsafe: switch to flow API object conversion function net/bonding: switch to flow API object conversion function
ethdev: deprecaterte_flow_copyfunction
ethdev: add missing item/actions to flow object converter
Quoted 1 line
Patch needs to be rebased to target v18.11 (in map file),
Right, will do it for v3.
Quoted 2 lines
and indeed new APIs
(rte_flow_conv) needs to be experimental.
This is what I did at first. Problem is that experimental APIs cannot be used in internal code without triggering a compilation error unless ALLOW_EXPERIMENTAL_API is defined (bonding cannot rely on an API marked as experimental).
Since this series reimplements rte_flow_copy() as a wrapper to rte_flow_conv(), I thought it didn't make sense for internal code to keep using the former either.
Considering this, shall I add -DDALLOW_EXPERIMENTAL_API to bonding PMD or keep things not experimental?
Quoted 1 line
And needs to remove deprecation notice in this patchset.
Doesn't it make sense to deprecate this function immediately after providing a replacement on top of which it is reimplemented? Users end up using the new function whether they want it or not. I don't think maintaining the old duplicated code around is the right thing to do either.
Quoted 1 line
Also do you think does make sense to announce this change in release notes?
I'm not sure it's worth a release note. It's a rather obscure helper function part of rte_flow. We didn't do it for rte_flow_copy() for instance. Please confirm if you think it's needed.
Quoted 1 line
Apart from above, any volunteer for reviewing actual implementation?
I hope Gaetan will take a look, he added rte_flow_copy() after all :)
Signature · 2 lines
Adrien Mazarguil 6WIND
Re: [PATCH v2 0/7] ethdev: add flow API object converter
In reply to Adrien MazarguilQuoted 28 lines · Adrien Mazarguil
This is a follow up to the "Flow API helpers enhancements" series submitted almost a year ago [1]. The new title is due to the reduced scope of this version.
rte_flow_conv()is a flexible replacement torte_flow_copy(), itself a temporary solution pending something better [2]. It replaces a lot of duplicated code found in testpmd and removes some of the maintenance burden that developers tend to forget (me included) when modifying pattern item or actions (updatingapp/test-pmd/config.cto be clear).This series was unearthed in order to complete the implementation of
RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE)in testpmd [3] without having to duplicate existing code once again.See individual patches for specific changes in this version.
v2 changes:
-
rte_flow_copy()is kept, albeit deprecated, no API/ABI impact.
- Updated bonding PMD.
- No more automatic generation ofrte_flow_conv.h.[1] https://mails.dpdk.org/archives/dev/2017-October/077551.html
[2] https://mails.dpdk.org/archives/dev/2017-July/070492.html
[3] Currently the command-line parser (cmdline_flow.c) is aware of these actions, howeverconfig.cisn't. Flow rules with such actions cannot be created and cannot be validated with PMDs that implement them.
Quoted 8 lines
Adrien Mazarguil (7):
ethdev: add flow API object converter
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function net/failsafe: switch to flow API object conversion function net/bonding: switch to flow API object conversion function
ethdev: deprecaterte_flow_copyfunction
ethdev: add missing item/actions to flow object converter
Causing build error for arm, it looks like related to rte_memcpy macro:
.../lib/librte_ethdev/rte_flow.c: In function ‘rte_flow_conv_item_spec’:.../lib/librte_ethdev/rte_flow.c:373:58: error: macro "rte_memcpy" passed 9 arguments, but takes just 3
(size > sizeof(*dst.raw) ? sizeof(*dst.raw) : size));
^Re: [PATCH v2 0/7] ethdev: add flow API object converter
Quoted 38 lines · Ferruh Yigit
Quoted 28 lines · Adrien Mazarguil
This is a follow up to the "Flow API helpers enhancements" series submitted almost a year ago [1]. The new title is due to the reduced scope of this version.
rte_flow_conv()is a flexible replacement torte_flow_copy(), itself a temporary solution pending something better [2]. It replaces a lot of duplicated code found in testpmd and removes some of the maintenance burden that developers tend to forget (me included) when modifying pattern item or actions (updatingapp/test-pmd/config.cto be clear).This series was unearthed in order to complete the implementation of
RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE)in testpmd [3] without having to duplicate existing code once again.See individual patches for specific changes in this version.
v2 changes:
-
rte_flow_copy()is kept, albeit deprecated, no API/ABI impact.
- Updated bonding PMD.
- No more automatic generation ofrte_flow_conv.h.[1] https://mails.dpdk.org/archives/dev/2017-October/077551.html
[2] https://mails.dpdk.org/archives/dev/2017-July/070492.html
[3] Currently the command-line parser (cmdline_flow.c) is aware of these actions, howeverconfig.cisn't. Flow rules with such actions cannot be created and cannot be validated with PMDs that implement them.Quoted 8 lines
Adrien Mazarguil (7):
ethdev: add flow API object converter
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function net/failsafe: switch to flow API object conversion function net/bonding: switch to flow API object conversion function
ethdev: deprecaterte_flow_copyfunction
ethdev: add missing item/actions to flow object converter
Quoted 6 lines
Causing build error for arm, it looks like related to
rte_memcpymacro:
.../lib/librte_ethdev/rte_flow.c: In function ‘rte_flow_conv_item_spec’:.../lib/librte_ethdev/rte_flow.c:373:58: error: macro "rte_memcpy" passed 9 arguments, but takes just 3(size > sizeof(*dst.raw) ? sizeof(*dst.raw) : size));
Thanks, noticed it after sending v2. I'll fix it for v3.
Signature · 2 lines
Adrien Mazarguil 6WIND
v3 cover ethdev: add flow API object converter
In reply to Adrien MazarguilThis is a follow up to the "Flow API helpers enhancements" series submitted almost a year ago [1]. The new title is due to the reduced scope of this version.
rte_flow_conv() is a flexible replacement to rte_flow_copy(), itself a temporary solution pending something better [2]. It replaces a lot of duplicated code found in testpmd and removes some of the maintenance burden that developers tend to forget (me included) when modifying pattern items or actions (updating app/test-pmd/config.c to be clear).
This series was unearthed in order to complete the implementation of RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE) in testpmd [3] without having to duplicate existing code once again.
See individual patches for specific changes in this version.
v3 changes:
- Marked rte_flow_conv() as experimental, modified net/bonding accordingly.
- Fixed compilation issue on ARM.
- Removed deprecation notice.
v2 changes:
- rte_flow_copy() is kept, albeit deprecated, no API/ABI impact.
- Updated bonding PMD.
- No more automatic generation of rte_flow_conv.h.
[1] https://mails.dpdk.org/archives/dev/2017-October/077551.html
[2] https://mails.dpdk.org/archives/dev/2017-July/070492.html
[3] Currently the command-line parser (cmdline_flow.c) is aware of these actions, however config.c isn't. Flow rules with such actions cannot be created and cannot be validated with PMDs that implement them.
Adrien Mazarguil (7):
ethdev: add flow API object converter
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function net/failsafe: switch to flow API object conversion function net/bonding: switch to flow API object conversion function
ethdev: add missing items/actions to flow object converter
ethdev: deprecate rte_flow_copy function
app/test-pmd/config.c | 407 +++------------ app/test-pmd/testpmd.h | 7 +- doc/guides/prog_guide/rte_flow.rst | 20 + doc/guides/rel_notes/deprecation.rst | 7 - drivers/net/bonding/Makefile | 1 + drivers/net/bonding/meson.build | 1 + drivers/net/bonding/rte_eth_bond_api.c | 6 +- drivers/net/bonding/rte_eth_bond_flow.c | 31 +- drivers/net/bonding/rte_eth_bond_private.h | 5 +- drivers/net/failsafe/failsafe_ether.c | 6 +- drivers/net/failsafe/failsafe_flow.c | 31 +- drivers/net/failsafe/failsafe_private.h | 5 +- lib/librte_ethdev/rte_ethdev_version.map | 1 + lib/librte_ethdev/rte_flow.c | 666 ++++++++++++++++++------ lib/librte_ethdev/rte_flow.h | 231 +++++++- 15 files changed, 886 insertions(+), 539 deletions(-)
Signature · 1 line
2.11.0
PATCH v3 1/7 ethdev: add flow API object converter
rte_flow_copy() is bound to duplicate flow rule descriptions (attributes, pattern and list of actions, all at once), however applications sometimes need more flexibility, for instance the ability to duplicate only one of the underlying objects (a single pattern item or action) or retrieve other properties such as their names.
Instead of adding dedicated functions to handle each possible use case, this patch introduces rte_flow_conv(), which supports any number of object conversion operations in an extensible manner.
This patch re-implements rte_flow_copy() as a wrapper to rte_flow_conv().
--
v3 changes:
- Worked around compilation issue on ARM where rte_memcpy() is a macro that chokes on commas.
- Marked rte_flow_conv() as experimental.
v2 changes:
- Modified patch to keep rte_flow_copy() around instead of removing it entirely. Reworded commit log accordingly.
- Moved failsafe PMD changes to a subsequent patch.
- Re-implemented rte_flow_copy() as a wrapper to rte_flow_conv() to reduce code duplication.
- Tweaked semantics of rte_flow_conv() to return the required number of bytes regardless of the size parameter; a buffer not large enough is not considered to be an error anymore. This change removes the need for a "store" pass in underlying helper functions.
- Renamed and properly documented internal helper functions.
--- doc/guides/prog_guide/rte_flow.rst | 19 + lib/librte_ethdev/rte_ethdev_version.map | 1 + lib/librte_ethdev/rte_flow.c | 553 ++++++++++++++++++-------- lib/librte_ethdev/rte_flow.h | 170 +++++++- 4 files changed, 582 insertions(+), 161 deletions(-)
doc/guides/prog_guide/rte_flow.rst +19 -0
@@ -2419,6 +2419,25 @@ This function initializes ``error`` (if non-NULL) with the provided parameters and sets ``rte_errno`` to ``code``. A negative error ``code`` is then returned. +Object conversion +~~~~~~~~~~~~~~~~~ + +.. code-block:: c + + int + rte_flow_conv(enum rte_flow_conv_op op, + void *dst, + size_t size, + const void *src, + struct rte_flow_error *error); + +Convert ``src`` to ``dst`` according to operation ``op``. Possible +operations include: + +- Attributes, pattern item or action duplication. +- Duplication of an entire pattern or list of actions. +- Duplication of a complete flow rule description. + Caveats -------
lib/librte_ethdev/rte_ethdev_version.map +1 -0
@@ -239,6 +239,7 @@ EXPERIMENTAL { rte_eth_dev_tx_offload_name; rte_eth_switch_domain_alloc; rte_eth_switch_domain_free; + rte_flow_conv; rte_flow_expand_rss; rte_mtr_capabilities_get; rte_mtr_create;
lib/librte_ethdev/rte_flow.c +396 -157
624 lines not on this page · raw message
lib/librte_ethdev/rte_flow.h +166 -4
200 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH v3 2/7 ethdev: add flow API item/action name conversion
In reply to Adrien MazarguilThis provides a means for applications to retrieve the name of flow pattern items and actions.
--
v2 changes:
- Replaced rte_flow_conv_name_ptr() with extra is_ptr argument to rte_flow_conv_name() since both functions were almost identical.
- Properly documented internal helper functions.
--- doc/guides/prog_guide/rte_flow.rst | 1 + lib/librte_ethdev/rte_flow.c | 63 +++++++++++++++++++++++++++++++++ lib/librte_ethdev/rte_flow.h | 56 +++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+)
doc/guides/prog_guide/rte_flow.rst +1 -0
@@ -2437,6 +2437,7 @@ operations include: - Attributes, pattern item or action duplication. - Duplication of an entire pattern or list of actions. - Duplication of a complete flow rule description. +- Pattern item or action name retrieval. Caveats -------
lib/librte_ethdev/rte_flow.c +63 -0
81 lines not on this page · raw message
lib/librte_ethdev/rte_flow.h +56 -0
62 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH v3 3/7 app/testpmd: rely on flow API conversion function
In reply to Adrien MazarguilThis commit replaces all local information about pattern items and actions as well as flow rule duplication code with calls to rte_flow_conv().
--- app/test-pmd/config.c | 407 +++++++------------------------------------- app/test-pmd/testpmd.h | 7 +- 2 files changed, 67 insertions(+), 347 deletions(-)
app/test-pmd/config.c +65 -342
@@ -984,324 +984,35 @@ port_mtu_set(portid_t port_id, uint16_t mtu)/* Generic flow management functions. */ -/** Generate flow_item[] entry. */ -#define MK_FLOW_ITEM (t, s) \ - [RTE_FLOW_ITEM_TYPE_ ## t] = { \ - .name = # t, \ - .size = s, \ - } - -/** Information about known flow pattern items. */ -static const struct { -const char *name; -size_t size; -} flow_item[] = { -MK_FLOW_ITEM (END ,0 ), -MK_FLOW_ITEM (VOID ,0 ), -MK_FLOW_ITEM (INVERT ,0 ), -MK_FLOW_ITEM (ANY ,sizeof (struct rte_flow_item_any )), -MK_FLOW_ITEM (PF ,0 ), -MK_FLOW_ITEM (VF ,sizeof (struct rte_flow_item_vf )), -MK_FLOW_ITEM (PHY_PORT ,sizeof (struct rte_flow_item_phy_port )), -MK_FLOW_ITEM (PORT_ID ,sizeof (struct rte_flow_item_port_id )), -MK_FLOW_ITEM (RAW ,sizeof (struct rte_flow_item_raw )), -MK_FLOW_ITEM (ETH ,sizeof (struct rte_flow_item_eth )), -MK_FLOW_ITEM (VLAN ,sizeof (struct rte_flow_item_vlan )), -MK_FLOW_ITEM (IPV4 ,sizeof (struct rte_flow_item_ipv4 )), -MK_FLOW_ITEM (IPV6 ,sizeof (struct rte_flow_item_ipv6 )), -MK_FLOW_ITEM (ICMP ,sizeof (struct rte_flow_item_icmp )), -MK_FLOW_ITEM (UDP ,sizeof (struct rte_flow_item_udp )), -MK_FLOW_ITEM (TCP ,sizeof (struct rte_flow_item_tcp )), -MK_FLOW_ITEM (SCTP ,sizeof (struct rte_flow_item_sctp )), -MK_FLOW_ITEM (VXLAN ,sizeof (struct rte_flow_item_vxlan )), -MK_FLOW_ITEM (E_TAG ,sizeof (struct rte_flow_item_e_tag )), -MK_FLOW_ITEM (NVGRE ,sizeof (struct rte_flow_item_nvgre )), -MK_FLOW_ITEM (MPLS ,sizeof (struct rte_flow_item_mpls )), -MK_FLOW_ITEM (GRE ,sizeof (struct rte_flow_item_gre )), -MK_FLOW_ITEM (FUZZY ,sizeof (struct rte_flow_item_fuzzy )), -MK_FLOW_ITEM (GTP ,sizeof (struct rte_flow_item_gtp )), -MK_FLOW_ITEM (GTPC ,sizeof (struct rte_flow_item_gtp )), -MK_FLOW_ITEM (GTPU ,sizeof (struct rte_flow_item_gtp )), -MK_FLOW_ITEM (GENEVE ,sizeof (struct rte_flow_item_geneve )), -MK_FLOW_ITEM (VXLAN_GPE ,sizeof (struct rte_flow_item_vxlan_gpe )), -MK_FLOW_ITEM (ARP_ETH_IPV4 ,sizeof (struct rte_flow_item_arp_eth_ipv4 )), -MK_FLOW_ITEM (IPV6_EXT ,sizeof (struct rte_flow_item_ipv6_ext )), -MK_FLOW_ITEM (ICMP6 ,sizeof (struct rte_flow_item_icmp6 )), -MK_FLOW_ITEM (ICMP6_ND_NS ,sizeof (struct rte_flow_item_icmp6_nd_ns )), -MK_FLOW_ITEM (ICMP6_ND_NA ,sizeof (struct rte_flow_item_icmp6_nd_na )), -MK_FLOW_ITEM (ICMP6_ND_OPT ,sizeof (struct rte_flow_item_icmp6_nd_opt )), -MK_FLOW_ITEM (ICMP6_ND_OPT_SLA_ETH , -sizeof (struct rte_flow_item_icmp6_nd_opt_sla_eth )), -MK_FLOW_ITEM (ICMP6_ND_OPT_TLA_ETH , -sizeof (struct rte_flow_item_icmp6_nd_opt_tla_eth )), -}; - -/** Pattern item specification types. */ -enum item_spec_type { -ITEM_SPEC , -ITEM_LAST , -ITEM_MASK , -}; - -/** Compute storage space needed by item specification and copy it. */ -static size_t -flow_item_spec_copy (void *buf,const struct rte_flow_item *item, -enum item_spec_type type) -{ -size_t size =0 ; -const void *data = - type ==ITEM_SPEC ? item->spec : - type ==ITEM_LAST ? item->last : - type ==ITEM_MASK ? item->mask : -NULL ; - -if (!item->spec || !data) - goto empty; -switch (item->type) { -union { -const struct rte_flow_item_raw *raw; - } spec; -union { -const struct rte_flow_item_raw *raw; - } last; -union { -const struct rte_flow_item_raw *raw; - } mask; -union { -const struct rte_flow_item_raw *raw; - } src; -union { -struct rte_flow_item_raw *raw; - } dst; -size_t off; - -case RTE_FLOW_ITEM_TYPE_RAW : - spec.raw = item->spec; - last.raw = item->last ? item->last : item->spec; - mask.raw = item->mask ? item->mask : &rte_flow_item_raw_mask; - src.raw = data; - dst.raw = buf; - off =RTE_ALIGN_CEIL (sizeof (struct rte_flow_item_raw ), -sizeof (*src.raw->pattern)); -if (type ==ITEM_SPEC || - (type ==ITEM_MASK && - ((spec.raw->length & mask.raw->length) >= - (last.raw->length & mask.raw->length)))) - size = spec.raw->length & mask.raw->length; -else - size = last.raw->length & mask.raw->length; - size = off + size *sizeof (*src.raw->pattern); -if (dst.raw) { -memcpy (dst.raw, src.raw,sizeof (*src.raw)); - dst.raw->pattern =memcpy ((uint8_t *)dst.raw + off, - src.raw->pattern, - size - off); - } -break ; -default : - size = flow_item[item->type].size; -if (buf) -memcpy (buf, data, size); -break ; - } -empty: -return RTE_ALIGN_CEIL (size,sizeof (double )); -} - -/** Generate flow_action[] entry. */ -#define MK_FLOW_ACTION (t, s) \ - [RTE_FLOW_ACTION_TYPE_ ## t] = { \ - .name = # t, \ - .size = s, \ - } - -/** Information about known flow actions. */ -static const struct { -const char *name; -size_t size; -} flow_action[] = { -MK_FLOW_ACTION (END ,0 ), -MK_FLOW_ACTION (VOID ,0 ), -MK_FLOW_ACTION (PASSTHRU ,0 ), -MK_FLOW_ACTION (MARK ,sizeof (struct rte_flow_action_mark )), -MK_FLOW_ACTION (FLAG ,0 ), -MK_FLOW_ACTION (QUEUE ,sizeof (struct rte_flow_action_queue )), -MK_FLOW_ACTION (DROP ,0 ), -MK_FLOW_ACTION (COUNT ,sizeof (struct rte_flow_action_count )), -MK_FLOW_ACTION (RSS ,sizeof (struct rte_flow_action_rss )), -MK_FLOW_ACTION (PF ,0 ), -MK_FLOW_ACTION (VF ,sizeof (struct rte_flow_action_vf )), -MK_FLOW_ACTION (PHY_PORT ,sizeof (struct rte_flow_action_phy_port )), -MK_FLOW_ACTION (PORT_ID ,sizeof (struct rte_flow_action_port_id )), -MK_FLOW_ACTION (METER ,sizeof (struct rte_flow_action_meter )), -MK_FLOW_ACTION (OF_SET_MPLS_TTL , -sizeof (struct rte_flow_action_of_set_mpls_ttl )), -MK_FLOW_ACTION (OF_DEC_MPLS_TTL ,0 ), -MK_FLOW_ACTION (OF_SET_NW_TTL , -sizeof (struct rte_flow_action_of_set_nw_ttl )), -MK_FLOW_ACTION (OF_DEC_NW_TTL ,0 ), -MK_FLOW_ACTION (OF_COPY_TTL_OUT ,0 ), -MK_FLOW_ACTION (OF_COPY_TTL_IN ,0 ), -MK_FLOW_ACTION (OF_POP_VLAN ,0 ), -MK_FLOW_ACTION (OF_PUSH_VLAN , -sizeof (struct rte_flow_action_of_push_vlan )), -MK_FLOW_ACTION (OF_SET_VLAN_VID , -sizeof (struct rte_flow_action_of_set_vlan_vid )), -MK_FLOW_ACTION (OF_SET_VLAN_PCP , -sizeof (struct rte_flow_action_of_set_vlan_pcp )), -MK_FLOW_ACTION (OF_POP_MPLS , -sizeof (struct rte_flow_action_of_pop_mpls )), -MK_FLOW_ACTION (OF_PUSH_MPLS , -sizeof (struct rte_flow_action_of_push_mpls )), -}; - -/** Compute storage space needed by action configuration and copy it. */ -static size_t -flow_action_conf_copy (void *buf,const struct rte_flow_action *action) -{ -size_t size =0 ; - -if (!action->conf) - goto empty; -switch (action->type) { -union { -const struct rte_flow_action_rss *rss; - } src; -union { -struct rte_flow_action_rss *rss; - } dst; -size_t off; - -case RTE_FLOW_ACTION_TYPE_RSS : - src.rss = action->conf; - dst.rss = buf; - off =0 ; -if (dst.rss) - *dst.rss = (struct rte_flow_action_rss ){ - .func = src.rss->func, - .level = src.rss->level, - .types = src.rss->types, - .key_len = src.rss->key_len, - .queue_num = src.rss->queue_num, - }; - off +=sizeof (*src.rss); -if (src.rss->key_len) { - off =RTE_ALIGN_CEIL (off,sizeof (double )); - size =sizeof (*src.rss->key) * src.rss->key_len; -if (dst.rss) - dst.rss->key =memcpy - ((void *)((uintptr_t )dst.rss + off), - src.rss->key, size); - off += size; - } -if (src.rss->queue_num) { - off =RTE_ALIGN_CEIL (off,sizeof (double )); - size =sizeof (*src.rss->queue) * src.rss->queue_num; -if (dst.rss) - dst.rss->queue =memcpy - ((void *)((uintptr_t )dst.rss + off), - src.rss->queue, size); - off += size; - } - size = off; -break ; -default : - size = flow_action[action->type].size; -if (buf) -memcpy (buf, action->conf, size); -break ; - } -empty: -return RTE_ALIGN_CEIL (size,sizeof (double )); -} -/** Generate a port_flow entry from attributes/pattern/actions. */ static struct port_flow *port_flow_new (const struct rte_flow_attr *attr,const struct rte_flow_item *pattern, -const struct rte_flow_action *actions) -{ -const struct rte_flow_item *item; -const struct rte_flow_action *action; -struct port_flow *pf =NULL ; -size_t tmp; -size_t off1 =0 ; -size_t off2 =0 ; -int err =ENOTSUP ; - -store: - item = pattern; -if (pf) - pf->pattern = (void *)&pf->data[off1]; -do { -struct rte_flow_item *dst =NULL ; - -if ((unsigned int )item->type >=RTE_DIM (flow_item) || - !flow_item[item->type].name) - goto notsup; -if (pf) - dst =memcpy (pf->data + off1, item,sizeof (*item)); - off1 +=sizeof (*item); -if (item->spec) { -if (pf) - dst->spec = pf->data + off2; - off2 +=flow_item_spec_copy - (pf ? pf->data + off2 :NULL , item,ITEM_SPEC ); - } -if (item->last) { -if (pf) - dst->last = pf->data + off2; - off2 +=flow_item_spec_copy - (pf ? pf->data + off2 :NULL , item,ITEM_LAST ); - } -if (item->mask) { -if (pf) - dst->mask = pf->data + off2; - off2 +=flow_item_spec_copy - (pf ? pf->data + off2 :NULL , item,ITEM_MASK ); - } - off2 =RTE_ALIGN_CEIL (off2,sizeof (double )); - }while ((item++)->type !=RTE_FLOW_ITEM_TYPE_END ); - off1 =RTE_ALIGN_CEIL (off1,sizeof (double )); - action = actions; -if (pf) - pf->actions = (void *)&pf->data[off1]; -do { -struct rte_flow_action *dst =NULL ; - -if ((unsigned int )action->type >=RTE_DIM (flow_action) || - !flow_action[action->type].name) - goto notsup; -if (pf) - dst =memcpy (pf->data + off1, action,sizeof (*action)); - off1 +=sizeof (*action); -if (action->conf) { -if (pf) - dst->conf = pf->data + off2; - off2 +=flow_action_conf_copy - (pf ? pf->data + off2 :NULL , action); - } - off2 =RTE_ALIGN_CEIL (off2,sizeof (double )); - }while ((action++)->type !=RTE_FLOW_ACTION_TYPE_END ); -if (pf !=NULL ) +const struct rte_flow_action *actions, +struct rte_flow_error *error) +{ +const struct rte_flow_conv_rule rule = { + .attr_ro = attr, + .pattern_ro = pattern, + .actions_ro = actions, + }; +struct port_flow *pf; +int ret; + + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE ,NULL ,0 , &rule, error); +if (ret <0 ) +return NULL ; + pf =calloc (1 , offsetof(struct port_flow , rule) + ret); +if (!pf) { +rte_flow_error_set + (error, errno,RTE_FLOW_ERROR_TYPE_UNSPECIFIED ,NULL , +"calloc() failed" ); +return NULL ; + } +if (rte_flow_conv (RTE_FLOW_CONV_OP_RULE , &pf->rule, ret, &rule, + error) >=0 )return pf; - off1 =RTE_ALIGN_CEIL (off1,sizeof (double )); - tmp =RTE_ALIGN_CEIL (offsetof(struct port_flow , data),sizeof (double )); - pf =calloc (1 , tmp + off1 + off2); -if (pf ==NULL ) - err = errno; -else { - *pf = (const struct port_flow ){ - .size = tmp + off1 + off2, - .attr = *attr, - }; - tmp -= offsetof(struct port_flow , data); - off2 = tmp + off1; - off1 = tmp; - goto store; - } -notsup: - rte_errno = err; +free (pf);return NULL ; }
@@ -1391,13 +1102,10 @@ port_flow_create(portid_t port_id, id = port->flow_list->id +1 ; }else id =0 ; - pf =port_flow_new (attr, pattern, actions); + pf =port_flow_new (attr, pattern, actions, &error);if (!pf) { -int err = rte_errno; - -printf ("Cannot allocate flow: %s\n" ,rte_strerror (err));rte_flow_destroy (port_id, flow,NULL ); -return -err; +return port_flow_complain (&error); } pf->next = port->flow_list; pf->id = id;
@@ -1489,6 +1197,7 @@ port_flow_query(portid_t port_id, uint32_t rule,union {struct rte_flow_query_count count; } query; +int ret;if (port_id_is_invalid (port_id,ENABLED_WARN ) || port_id == (portid_t )RTE_PORT_ALL )
@@ -1501,11 +1210,10 @@ port_flow_query(portid_t port_id, uint32_t rule,printf ("Flow rule #%u not found\n" , rule);return -ENOENT ; } -if ((unsigned int )action->type >=RTE_DIM (flow_action) || - !flow_action[action->type].name) - name ="unknown" ; -else - name = flow_action[action->type].name; + ret =rte_flow_conv (RTE_FLOW_CONV_OP_ACTION_NAME_PTR , + &name,sizeof (name), action, &error); +if (ret <0 ) +return port_flow_complain (&error);switch (action->type) {case RTE_FLOW_ACTION_TYPE_COUNT :break ;
@@ -1558,48 +1266,63 @@ port_flow_list(portid_t port_id, uint32_t n, const uint32_t group[n])/* Sort flows by group, priority and ID. */ for (pf = port->flow_list; pf !=NULL ; pf = pf->next) {struct port_flow **tmp; +const struct rte_flow_attr *curr = pf->rule.attr;if (n) {/* Filter out unwanted groups. */ for (i =0 ; i != n; ++i) -if (pf->attr.group == group[i]) +if (curr->group == group[i])break ;if (i == n)continue ; } - tmp = &list; -while (*tmp && - (pf->attr.group > (*tmp)->attr.group || - (pf->attr.group == (*tmp)->attr.group && - pf->attr.priority > (*tmp)->attr.priority) || - (pf->attr.group == (*tmp)->attr.group && - pf->attr.priority == (*tmp)->attr.priority && - pf->id > (*tmp)->id))) - tmp = &(*tmp)->tmp; +for (tmp = &list; *tmp; tmp = &(*tmp)->tmp) { +const struct rte_flow_attr *comp = (*tmp)->rule.attr; + +if (curr->group > comp->group || + (curr->group == comp->group && + curr->priority > comp->priority) || + (curr->group == comp->group && + curr->priority == comp->priority && + pf->id > (*tmp)->id)) +continue ; +break ; + } pf->tmp = *tmp; *tmp = pf; }printf ("ID\tGroup\tPrio\tAttr\tRule\n" );for (pf = list; pf !=NULL ; pf = pf->tmp) { -const struct rte_flow_item *item = pf->pattern; -const struct rte_flow_action *action = pf->actions; +const struct rte_flow_item *item = pf->rule.pattern; +const struct rte_flow_action *action = pf->rule.actions; +const char *name;printf ("%" PRIu32"\t%" PRIu32"\t%" PRIu32"\t%c%c%c\t" , pf->id, - pf->attr.group, - pf->attr.priority, - pf->attr.ingress ?'i' :'-' , - pf->attr.egress ?'e' :'-' , - pf->attr.transfer ?'t' :'-' ); + pf->rule.attr->group, + pf->rule.attr->priority, + pf->rule.attr->ingress ?'i' :'-' , + pf->rule.attr->egress ?'e' :'-' , + pf->rule.attr->transfer ?'t' :'-' );while (item->type !=RTE_FLOW_ITEM_TYPE_END ) { +if (rte_flow_conv (RTE_FLOW_CONV_OP_ITEM_NAME_PTR , + &name,sizeof (name), + (void *)(uintptr_t )item->type, +NULL ) <=0 ) + name ="[UNKNOWN]" ;if (item->type !=RTE_FLOW_ITEM_TYPE_VOID ) -printf ("%s " , flow_item[item->type].name); +printf ("%s " , name); ++item; }printf ("=>" );while (action->type !=RTE_FLOW_ACTION_TYPE_END ) { +if (rte_flow_conv (RTE_FLOW_CONV_OP_ACTION_NAME_PTR , + &name,sizeof (name), + (void *)(uintptr_t )action->type, +NULL ) <=0 ) + name ="[UNKNOWN]" ;if (action->type !=RTE_FLOW_ACTION_TYPE_VOID ) -printf (" %s" , flow_action[action->type].name); +printf (" %s" , name); ++action; }printf ("\n" );
app/test-pmd/testpmd.h +2 -5
@@ -124,15 +124,12 @@ struct fwd_stream {/** Descriptor for a single flow. */ struct port_flow { -size_t size;/**< Allocated space including data[]. */ struct port_flow *next;/**< Next flow in list. */ struct port_flow *tmp;/**< Temporary linking. */ uint32_t id;/**< Flow rule ID. */ struct rte_flow *flow;/**< Opaque flow object returned by PMD. */ -struct rte_flow_attr attr;/**< Attributes. */ -struct rte_flow_item *pattern;/**< Pattern. */ -struct rte_flow_action *actions;/**< Actions. */ -uint8_t data[];/**< Storage for pattern/actions. */ +struct rte_flow_conv_rule rule;/* Saved flow rule description. */ +uint8_t data[];/**< Storage for flow rule description */ };#ifdef SOFTNIC
Signature · 1 line
2.11.0
PATCH v3 4/7 net/failsafe: switch to flow API object conversion function
In reply to Adrien MazarguilThis patch replaces rte_flow_copy() with rte_flow_conv().
--
v2 changes:
- Patch was split from "ethdev: add flow API object converter".
--- drivers/net/failsafe/failsafe_ether.c | 6 +++--- drivers/net/failsafe/failsafe_flow.c | 31 +++++++++++++++++++++------- drivers/net/failsafe/failsafe_private.h | 5 ++++- 3 files changed, 31 insertions(+), 11 deletions(-)
drivers/net/failsafe/failsafe_ether.c +3 -3
@@ -230,9 +230,9 @@ fs_eth_dev_conf_apply(struct rte_eth_dev *dev,DEBUG ("Creating flow #%" PRIu32, i++); flow->flows[SUB_ID (sdev)] =rte_flow_create (PORT_ID (sdev), - &flow->fd->attr, - flow->fd->items, - flow->fd->actions, + flow->rule.attr, + flow->rule.pattern, + flow->rule.actions, &ferror); ret = rte_errno;if (ret)
drivers/net/failsafe/failsafe_flow.c +24 -7
@@ -3,8 +3,11 @@* Copyright 2017 Mellanox Technologies, Ltd */ +#include <stddef.h> +#include <string.h> #include <sys/queue.h> +#include <rte_errno.h> #include <rte_malloc.h> #include <rte_tailq.h> #include <rte_flow.h>
@@ -18,19 +21,33 @@ fs_flow_allocate(const struct rte_flow_attr *attr, const struct rte_flow_action *actions) {struct rte_flow *flow; -size_t fdsz; +const struct rte_flow_conv_rule rule = { + .attr_ro = attr, + .pattern_ro = items, + .actions_ro = actions, + }; +struct rte_flow_error error; +int ret; - fdsz =rte_flow_copy (NULL ,0 , attr, items, actions); - flow =rte_zmalloc (NULL , -sizeof (struct rte_flow ) + fdsz, + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE ,NULL ,0 , &rule, &error); +if (ret <0 ) { +ERROR ("Unable to process flow rule (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno)); +return NULL ; + } + flow =rte_zmalloc (NULL , offsetof(struct rte_flow , rule) + ret,RTE_CACHE_LINE_SIZE );if (flow ==NULL ) {ERROR ("Could not allocate new flow" );return NULL ; } - flow->fd = (void *)((uintptr_t )flow +sizeof (*flow)); -if (rte_flow_copy (flow->fd, fdsz, attr, items, actions) != fdsz) { -ERROR ("Failed to copy flow description" ); + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE , &flow->rule, ret, &rule, + &error); +if (ret <0 ) { +ERROR ("Failed to copy flow rule (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno));rte_free (flow);return NULL ; }
drivers/net/failsafe/failsafe_private.h +4 -1
@@ -6,6 +6,7 @@#ifndef _RTE_ETH_FAILSAFE_PRIVATE_H_#define _RTE_ETH_FAILSAFE_PRIVATE_H_ +#include <stdint.h> #include <sys/queue.h> #include <pthread.h>
@@ -13,6 +14,7 @@#include <rte_dev.h> #include <rte_ethdev_driver.h> #include <rte_devargs.h> +#include <rte_flow.h> #include <rte_interrupts.h> #define FAILSAFE_DRIVER_NAME "Fail-safe PMD"
@@ -81,7 +83,8 @@ struct rte_flow {/* sub_flows */ struct rte_flow *flows[FAILSAFE_MAX_ETHPORTS ];/* flow description for synchronization */ -struct rte_flow_desc *fd; +struct rte_flow_conv_rule rule; +uint8_t rule_data[]; };enum dev_state {
Signature · 1 line
2.11.0
PATCH v3 5/7 net/bonding: switch to flow API object conversion function
In reply to Adrien MazarguilThis patch replaces rte_flow_copy() with rte_flow_conv().
--
v3 changes:
- Added build directives to allow experimental APIs, now needed for rte_flow_conv().
v2 changes:
- Patch was not present in original series.
--- drivers/net/bonding/Makefile | 1 + drivers/net/bonding/meson.build | 1 + drivers/net/bonding/rte_eth_bond_api.c | 6 ++--- drivers/net/bonding/rte_eth_bond_flow.c | 31 +++++++++++++++++++------ drivers/net/bonding/rte_eth_bond_private.h | 5 +++- 5 files changed, 33 insertions(+), 11 deletions(-)
drivers/net/bonding/Makefile +1 -0
@@ -8,6 +8,7 @@ include $(RTE_SDK)/mk/rte.vars.mk # LIB = librte_pmd_bond.a +CFLAGS += -DALLOW_EXPERIMENTAL_API CFLAGS += -O3 CFLAGS += $(WERROR_FLAGS) LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
drivers/net/bonding/meson.build +1 -0
@@ -3,6 +3,7 @@ name = 'bond' #, james bond :-) version = 2 +allow_experimental_apis = true sources = files('rte_eth_bond_api.c', 'rte_eth_bond_pmd.c', 'rte_eth_bond_flow.c', 'rte_eth_bond_args.c', 'rte_eth_bond_8023ad.c', 'rte_eth_bond_alb.c')
drivers/net/bonding/rte_eth_bond_api.c +3 -3
@@ -245,9 +245,9 @@ slave_rte_flow_prepare(uint16_t slave_id, struct bond_dev_private *internals) } TAILQ_FOREACH(flow, &internals->flow_list, next) { flow->flows[slave_id] =rte_flow_create (slave_port_id, - &flow->fd->attr, - flow->fd->items, - flow->fd->actions, + flow->rule.attr, + flow->rule.pattern, + flow->rule.actions, &ferror);if (flow->flows[slave_id] ==NULL ) { RTE_BOND_LOG(ERR, "Cannot create flow for slave"
drivers/net/bonding/rte_eth_bond_flow.c +24 -7
@@ -2,8 +2,11 @@* Copyright 2018 Mellanox Technologies, Ltd */ +#include <stddef.h> +#include <string.h> #include <sys/queue.h> +#include <rte_errno.h> #include <rte_malloc.h> #include <rte_tailq.h> #include <rte_flow.h>
@@ -16,19 +19,33 @@ bond_flow_alloc(int numa_node, const struct rte_flow_attr *attr, const struct rte_flow_action *actions) {struct rte_flow *flow; -size_t fdsz; +const struct rte_flow_conv_rule rule = { + .attr_ro = attr, + .pattern_ro = items, + .actions_ro = actions, + }; +struct rte_flow_error error; +int ret; - fdsz =rte_flow_copy (NULL ,0 , attr, items, actions); - flow =rte_zmalloc_socket (NULL ,sizeof (struct rte_flow ) + fdsz, + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE ,NULL ,0 , &rule, &error); +if (ret <0 ) { +RTE_BOND_LOG (ERR ,"Unable to process flow rule (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno)); +return NULL ; + } + flow =rte_zmalloc_socket (NULL , offsetof(struct rte_flow , rule) + ret,RTE_CACHE_LINE_SIZE , numa_node);if (unlikely (flow ==NULL )) {RTE_BOND_LOG (ERR ,"Could not allocate new flow" );return NULL ; } - flow->fd = (void *)((uintptr_t )flow +sizeof (*flow)); -if (unlikely (rte_flow_copy (flow->fd, fdsz, attr, items, actions) != - fdsz)) { -RTE_BOND_LOG (ERR ,"Failed to copy flow description" ); + ret =rte_flow_conv (RTE_FLOW_CONV_OP_RULE , &flow->rule, ret, &rule, + &error); +if (ret <0 ) { +RTE_BOND_LOG (ERR ,"Failed to copy flow rule (%s): %s" , + error.message ? error.message :"unspecified" , +strerror (rte_errno));rte_free (flow);return NULL ; }
drivers/net/bonding/rte_eth_bond_private.h +4 -1
@@ -5,9 +5,11 @@#ifndef _RTE_ETH_BOND_PRIVATE_H_#define _RTE_ETH_BOND_PRIVATE_H_ +#include <stdint.h> #include <sys/queue.h> #include <rte_ethdev_driver.h> +#include <rte_flow.h> #include <rte_spinlock.h> #include <rte_bitmap.h> #include <rte_flow_driver.h>
@@ -93,7 +95,8 @@ struct rte_flow {/* Slaves flows */ struct rte_flow *flows[RTE_MAX_ETHPORTS ];/* Flow description for synchronization */ -struct rte_flow_desc *fd; +struct rte_flow_conv_rule rule; +uint8_t rule_data[]; }; typedef void (*burst_xmit_hash_t)(struct rte_mbuf **buf, uint16_t nb_pkts,
Signature · 1 line
2.11.0
PATCH v3 6/7 ethdev: add missing items/actions to flow object converter
In reply to Adrien MazarguilSeveral pattern items and actions were never handled by rte_flow_copy() because their descriptions were missing. rte_flow_conv() inherited this deficiency.
This patch adds them and reorders others to match rte_flow.h. It doesn't pose as a fix because so far no one has complained about it and rte_flow_conv() would have to be backported as well: this function is the only sane approach to handle VXLAN and NVGRE encap definitions.
As a matter of fact, it's the last missing piece to finally allow testpmd users to request the creation of VXLAN/NVGRE encap/decap flow rules without getting rejected outright.
--
v2 changes:
- Patch was not present in original series.
--- lib/librte_ethdev/rte_flow.c | 50 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)
lib/librte_ethdev/rte_flow.c +48 -2
@@ -51,10 +51,15 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {MK_FLOW_ITEM (TCP ,sizeof (struct rte_flow_item_tcp )),MK_FLOW_ITEM (SCTP ,sizeof (struct rte_flow_item_sctp )),MK_FLOW_ITEM (VXLAN ,sizeof (struct rte_flow_item_vxlan )), -MK_FLOW_ITEM (MPLS ,sizeof (struct rte_flow_item_mpls )), -MK_FLOW_ITEM (GRE ,sizeof (struct rte_flow_item_gre )),MK_FLOW_ITEM (E_TAG ,sizeof (struct rte_flow_item_e_tag )),MK_FLOW_ITEM (NVGRE ,sizeof (struct rte_flow_item_nvgre )), +MK_FLOW_ITEM (MPLS ,sizeof (struct rte_flow_item_mpls )), +MK_FLOW_ITEM (GRE ,sizeof (struct rte_flow_item_gre )), +MK_FLOW_ITEM (FUZZY ,sizeof (struct rte_flow_item_fuzzy )), +MK_FLOW_ITEM (GTP ,sizeof (struct rte_flow_item_gtp )), +MK_FLOW_ITEM (GTPC ,sizeof (struct rte_flow_item_gtp )), +MK_FLOW_ITEM (GTPU ,sizeof (struct rte_flow_item_gtp )), +MK_FLOW_ITEM (ESP ,sizeof (struct rte_flow_item_esp )),MK_FLOW_ITEM (GENEVE ,sizeof (struct rte_flow_item_geneve )),MK_FLOW_ITEM (VXLAN_GPE ,sizeof (struct rte_flow_item_vxlan_gpe )), MK_FLOW_ITEM(ARP_ETH_IPV4, sizeof(struct rte_flow_item_arp_eth_ipv4)),
@@ -67,6 +72,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = { sizeof(struct rte_flow_item_icmp6_nd_opt_sla_eth)),MK_FLOW_ITEM (ICMP6_ND_OPT_TLA_ETH ,sizeof (struct rte_flow_item_icmp6_nd_opt_tla_eth )), + MK_FLOW_ITEM(MARK, sizeof(struct rte_flow_item_mark)), };/** Generate flow_action[] entry. */
@@ -81,6 +87,7 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = {MK_FLOW_ACTION (END ,0 ),MK_FLOW_ACTION (VOID ,0 ),MK_FLOW_ACTION (PASSTHRU ,0 ), +MK_FLOW_ACTION (JUMP ,sizeof (struct rte_flow_action_jump )),MK_FLOW_ACTION (MARK ,sizeof (struct rte_flow_action_mark )),MK_FLOW_ACTION (FLAG ,0 ), MK_FLOW_ACTION(QUEUE, sizeof(struct rte_flow_action_queue)),
@@ -91,6 +98,8 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = {MK_FLOW_ACTION (VF ,sizeof (struct rte_flow_action_vf )),MK_FLOW_ACTION (PHY_PORT ,sizeof (struct rte_flow_action_phy_port )),MK_FLOW_ACTION (PORT_ID ,sizeof (struct rte_flow_action_port_id )), +MK_FLOW_ACTION (METER ,sizeof (struct rte_flow_action_meter )), +MK_FLOW_ACTION (SECURITY ,sizeof (struct rte_flow_action_security )),MK_FLOW_ACTION (OF_SET_MPLS_TTL ,sizeof (struct rte_flow_action_of_set_mpls_ttl )), MK_FLOW_ACTION(OF_DEC_MPLS_TTL, 0),
@@ -110,6 +119,10 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = { sizeof(struct rte_flow_action_of_pop_mpls)),MK_FLOW_ACTION (OF_PUSH_MPLS ,sizeof (struct rte_flow_action_of_push_mpls )), +MK_FLOW_ACTION (VXLAN_ENCAP ,sizeof (struct rte_flow_action_vxlan_encap )), +MK_FLOW_ACTION (VXLAN_DECAP ,0 ), +MK_FLOW_ACTION (NVGRE_ENCAP ,sizeof (struct rte_flow_action_vxlan_encap )), + MK_FLOW_ACTION(NVGRE_DECAP, 0), }; static int
@@ -407,11 +420,16 @@ rte_flow_conv_action_conf(void *buf, const size_t size,switch (action->type) {union {const struct rte_flow_action_rss *rss; +const struct rte_flow_action_vxlan_encap *vxlan_encap; +const struct rte_flow_action_nvgre_encap *nvgre_encap; } src;union {struct rte_flow_action_rss *rss; +struct rte_flow_action_vxlan_encap *vxlan_encap; +struct rte_flow_action_nvgre_encap *nvgre_encap; } dst;size_t tmp; +int ret;case RTE_FLOW_ACTION_TYPE_RSS : src.rss = action->conf;
@@ -445,6 +463,34 @@ rte_flow_conv_action_conf(void *buf, const size_t size, off += tmp; }break ; +case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP : +case RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP : + src.vxlan_encap = action->conf; + dst.vxlan_encap = buf; +RTE_BUILD_BUG_ON (sizeof (*src.vxlan_encap) != +sizeof (*src.nvgre_encap) || + offsetof(struct rte_flow_action_vxlan_encap , + definition) != + offsetof(struct rte_flow_action_nvgre_encap , + definition)); + off =sizeof (*dst.vxlan_encap); +if (src.vxlan_encap->definition) { + off =RTE_ALIGN_CEIL + (off,sizeof (*dst.vxlan_encap->definition)); + ret =rte_flow_conv + (RTE_FLOW_CONV_OP_PATTERN , + (void *)((uintptr_t )dst.vxlan_encap + off), + size > off ? size - off :0 , + src.vxlan_encap->definition,NULL ); +if (ret <0 ) +return 0 ; +if (size >= off + ret) + dst.vxlan_encap->definition = + (void *)((uintptr_t )dst.vxlan_encap + + off); + off += ret; + } +break ;default : off = rte_flow_desc_action[action->type].size;rte_memcpy (buf, action->conf, (size > off ? off : size));
Signature · 1 line
2.11.0
PATCH v3 7/7 ethdev: deprecate rte_flow_copy function
In reply to Adrien MazarguilNo users left for this function, time to deprecate it.
--
v3 changes:
- Removed deprecation notice (finally got Ferruh's point), made patch last in series.
v2 changes:
- Patch was not present in original series.
--- doc/guides/rel_notes/deprecation.rst | 7 ------- lib/librte_ethdev/rte_flow.h | 7 ++++++- 2 files changed, 6 insertions(+), 8 deletions(-)
doc/guides/rel_notes/deprecation.rst +0 -7
@@ -88,10 +88,3 @@ Deprecation Notices - ``rte_pdump_set_socket_dir`` will be removed; - The parameter, ``path``, of ``rte_pdump_init`` will be removed; - The enum ``rte_pdump_socktype`` will be removed. - -* ethdev: flow API function ``rte_flow_copy()`` will be deprecated in v18.11 - in favor of ``rte_flow_conv()`` (which will appear in that version) and - subsequently removed for v19.02. - - This is due to a lack of flexibility and reliance on a type unusable with - C++ programs (struct rte_flow_desc).
lib/librte_ethdev/rte_flow.h +6 -1
@@ -2332,6 +2332,7 @@ rte_flow_error_set(struct rte_flow_error *error, const char *message);/** +* @deprecated * @see rte_flow_copy() */ struct rte_flow_desc {
@@ -2343,10 +2344,13 @@ struct rte_flow_desc { };/** +* @deprecated * Copy an rte_flow rule description. * * This interface is kept for compatibility with older applications but is -* implemented as a wrapper to rte_flow_conv(). +* implemented as a wrapper to rte_flow_conv(). It is deprecated due to its +* lack of flexibility and reliance on a type unusable with C++ programs +* (struct rte_flow_desc). * * @param[in] fd * Flow rule description.
@@ -2365,6 +2369,7 @@ struct rte_flow_desc {* If len is lower than the size of the flow, the number of bytes that would * have been written to desc had it been sufficient. Nothing is written. */ +__rte_deprecated size_t rte_flow_copy(struct rte_flow_desc *fd, size_t len, const struct rte_flow_attr *attr,
Signature · 1 line
2.11.0
Re: [PATCH v3 7/7] ethdev: deprecate rte_flow_copy function
Quoted 65 lines · Adrien Mazarguil
No users left for this function, time to deprecate it.
Signed-off-by Adrien Mazarguil <[email protected]>Cc Thomas Monjalon <[email protected]>Cc Ferruh Yigit <[email protected]>Cc Andrew Rybchenko <[email protected]>Cc Gaetan Rivet <[email protected]>--
v3 changes:- Removed deprecation notice (finally got Ferruh's point), made patch last in series.
v2 changes:
- Patch was not present in original series.
--- doc/guides/rel_notes/deprecation.rst | 7 ------- lib/librte_ethdev/rte_flow.h | 7 ++++++- 2 files changed, 6 insertions(+), 8 deletions(-)doc/guides/rel_notes/deprecation.rst +0
-7@@ -88,10 +88,3 @@ Deprecation Notices - ``rte_pdump_set_socket_dir`` will be removed; - The parameter, ``path``, of ``rte_pdump_init`` will be removed; - The enum ``rte_pdump_socktype`` will be removed. - -* ethdev: flow API function ``rte_flow_copy()`` will be deprecated in v18.11 - in favor of ``rte_flow_conv()`` (which will appear in that version) and - subsequently removed for v19.02. - - This is due to a lack of flexibility and reliance on a type unusable with - C++ programs (struct rte_flow_desc).lib/librte_ethdev/rte_flow.h +5
-1@@ -2332,6 +2332,7 @@ rte_flow_error_set(struct rte_flow_error *error, const char *message);/** +* @deprecated * @see rte_flow_copy() */ struct rte_flow_desc {@@ -2343,10 +2344,13 @@ struct rte_flow_desc { };/** +* @deprecated * Copy an rte_flow rule description. * * This interface is kept for compatibility with older applications but is -* implemented as a wrapper to rte_flow_conv(). +* implemented as a wrapper to rte_flow_conv(). It is deprecated due to its +* lack of flexibility and reliance on a type unusable with C++ programs +* (struct rte_flow_desc). * * @param[in] fd * Flow rule description.
lib/librte_ethdev/rte_flow.h struct rte_flow_desc { · line 2375
Quoted 7 lines
lib/librte_ethdev/rte_flow.h +1
-0@@ -2365,6 +2369,7 @@ struct rte_flow_desc {* If len is lower than the size of the flow, the number of bytes that would * have been written to desc had it been sufficient. Nothing is written. */ +__rte_deprecated size_t rte_flow_copy(struct rte_flow_desc *fd, size_t len, const struct rte_flow_attr *attr,
Not exactly related to this patch but I have a deprecation process question, what we are mostly doing is:
1) Release N, document in deprecation.rst the API that will be changed
2) Release N + 1, change the API and remove the note from the deprecation.rst
But using __rte_deprecated makes sense, how can we include this into process?
It looks like we can use __rte_deprecated only when an API replaced (add a new one & deprecate old one), but if an API changed switch needs to be atomic.
Options I can think of:
For replacing an API,
A)
a1) Release N, add note to deprecation.rst and add __rte_deprecated to old API a2) Release N + 1, add new API, remove note from deprecation.rst and remove old API
B)
b1) Release N, add note to deprecation.rst
b2) Release N + 1, remove note from deprecation.rst add __rte_deprecated to old API and add new API (as non experimental [1])
b3) Release N + 1 + M, remove old API (perhaps cleanup on each new LTS)
For changing exiting API,
C)
c1) Release N, add note to deprecation.rst
c2) Release N + 1, remove note from deprecation.rst and switch to new API
The problem with C) is, even developer sees the deprecation note, there is nothing she can do or prepare, only in "Release N + 1" she will hit to the change and will update her code. Not very useful for developer, so what about:
D)
d1) Release N, add note to deprecation.rst implement new API within RTE_NEXT_ABI #ifdef
d2) Release N + 1, remove note from deprecation.rst and switch to new API
Switching to D means one can't send deprecation notice without doing the actual implementation, so there is a big difference between C).
I am for B & D, what do you think?
[1]
This is happening again with rte_flow_conv(), old API is deprecated, new API is experimental, from user point of view there is no stable API.
I am not happy with "an API should be experimental for first release it has been introduced" policy, is it really helping, can we re-visit this again?
Re: [PATCH v3 0/7] ethdev: add flow API object converter
In reply to Adrien MazarguilQuoted 43 lines · Adrien Mazarguil
This is a follow up to the "Flow API helpers enhancements" series submitted almost a year ago [1]. The new title is due to the reduced scope of this version.
rte_flow_conv()is a flexible replacement torte_flow_copy(), itself a temporary solution pending something better [2]. It replaces a lot of duplicated code found in testpmd and removes some of the maintenance burden that developers tend to forget (me included) when modifying pattern items or actions (updatingapp/test-pmd/config.cto be clear).This series was unearthed in order to complete the implementation of
RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE)in testpmd [3] without having to duplicate existing code once again.See individual patches for specific changes in this version.
v3 changes:
- Marked
rte_flow_conv()as experimental, modified net/bonding accordingly.
- Fixed compilation issue on ARM.
- Removed deprecation notice.v2 changes:
-
rte_flow_copy()is kept, albeit deprecated, no API/ABI impact.
- Updated bonding PMD.
- No more automatic generation ofrte_flow_conv.h.[1] https://mails.dpdk.org/archives/dev/2017-October/077551.html
[2] https://mails.dpdk.org/archives/dev/2017-July/070492.html
[3] Currently the command-line parser (cmdline_flow.c) is aware of these actions, howeverconfig.cisn't. Flow rules with such actions cannot be created and cannot be validated with PMDs that implement them.Adrien Mazarguil (7):
ethdev: add flow API object converter
ethdev: add flow API item/action name conversion app/testpmd: rely on flow API conversion function net/failsafe: switch to flow API object conversion function net/bonding: switch to flow API object conversion function
ethdev: add missing items/actions to flow object converter
ethdev: deprecaterte_flow_copyfunction
Quoted 19 lines
app/test-pmd/config.c | 407 +++------------ app/test-pmd/testpmd.h | 7 +- doc/guides/prog_guide/rte_flow.rst | 20 + doc/guides/rel_notes/deprecation.rst | 7 - drivers/net/bonding/Makefile | 1 + drivers/net/bonding/meson.build | 1 + drivers/net/bonding/rte_eth_bond_api.c | 6 +- drivers/net/bonding/rte_eth_bond_flow.c | 31 +- drivers/net/bonding/rte_eth_bond_private.h | 5 +- drivers/net/failsafe/failsafe_ether.c | 6 +- drivers/net/failsafe/failsafe_flow.c | 31 +- drivers/net/failsafe/failsafe_private.h | 5 +- lib/librte_ethdev/rte_ethdev_version.map | 1 + lib/librte_ethdev/rte_flow.c | 666 ++++++++++++++++++------ lib/librte_ethdev/rte_flow.h | 231 +++++++- 15 files changed, 886 insertions(+), 539 deletions(-)Signature · 1 line
2.11.0
Signature · 2 lines
Nélio Laranjeiro 6WIND
Re: [PATCH v3 0/7] ethdev: add flow API object converter
31/08/2018 13:32, Nélio Laranjeiro:
Quoted 11 lines · Adrien Mazarguil
Quoted 10 lines
This is a follow up to the "Flow API helpers enhancements" series submitted almost a year ago [1]. The new title is due to the reduced scope of this version.
rte_flow_conv()is a flexible replacement torte_flow_copy(), itself a temporary solution pending something better [2]. It replaces a lot of duplicated code found in testpmd and removes some of the maintenance burden that developers tend to forget (me included) when modifying pattern items or actions (updatingapp/test-pmd/config.cto be clear).
Quoted 5 lines
Quoted 3 lines
This series was unearthed in order to complete the implementation of
RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE)in testpmd [3] without having to duplicate existing code once again.Acked-by Nelio Laranjeiro <[email protected]>
No more comment after one month.
Ferruh, I think it should go into next-net.
Re: [PATCH v3 0/7] ethdev: add flow API object converter
Quoted 12 lines · Thomas Monjalon
31/08/2018 13:32, Nélio Laranjeiro:
Quoted 11 lines · Adrien Mazarguil
Quoted 10 lines
This is a follow up to the "Flow API helpers enhancements" series submitted almost a year ago [1]. The new title is due to the reduced scope of this version.
rte_flow_conv()is a flexible replacement torte_flow_copy(), itself a temporary solution pending something better [2]. It replaces a lot of duplicated code found in testpmd and removes some of the maintenance burden that developers tend to forget (me included) when modifying pattern items or actions (updatingapp/test-pmd/config.cto be clear).
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
This series was unearthed in order to complete the implementation of
RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE)in testpmd [3] without having to duplicate existing code once again.Acked-by Nelio Laranjeiro <[email protected]>No more comment after one month.
Ferruh, I think it should go into next-net.
Series applied to dpdk-next-net/master, thanks.