<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word"> <div class="WordSection1"> Hi,<o:p></o:p> For some reason this mail stopped being plain text.<o:p></o:p> Pleas find my comment marked with [Ori]<o:p></o:p> <o:p> </o:p> Best,<o:p></o:p> Ori<o:p></o:p> <o:p> </o:p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Jack Min <jackmin@nvidia.com> Sent: Thursday, June 2, 2022 1:24 PM To: Ori Kam <orika@nvidia.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@xilinx.com> Cc: dev@dpdk.org Subject: Re: [RFC v2 2/2] ethdev: queue-based flow aged report<o:p></o:p> </div> </div> <o:p> </o:p> <div> On 6/2/22 14:10, Ori Kam wrote:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>Hi,<o:p></o:p></pre> </blockquote> Hello, <o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre><o:p> </o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>-----Original Message-----<o:p></o:p></pre> <pre>From: Andrew Rybchenko <a href="mailto:andrew.rybchenko@oktetlabs.ru"><andrew.rybchenko@oktetlabs.ru></a><o:p></o:p></pre> <pre>Sent: Wednesday, June 1, 2022 9:21 PM<o:p></o:p></pre> <pre>Subject: Re: [RFC v2 2/2] ethdev: queue-based flow aged report<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>Again, summary must not be a statement.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>On 6/1/22 10:39, Xiaoyu Min wrote:<o:p></o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>When application use queue-based flow rule management and operate the<o:p></o:p></pre> <pre>same flow rule on the same queue, e.g create/destroy/query, API of<o:p></o:p></pre> <pre>querying aged flow rules should also have queue id parameter just like<o:p></o:p></pre> <pre>other queue-based flow APIs.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>By this way, PMD can work in more optimized way since resources are<o:p></o:p></pre> <pre>isolated by queue and needn't synchronize.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>If application do use queue-based flow management but configure port<o:p></o:p></pre> <pre>without RTE_FLOW_PORT_FLAG_STRICT_QUEUE, which means application operate<o:p></o:p></pre> <pre>a given flow rule on different queues, the queue id parameter will<o:p></o:p></pre> <pre>be ignored.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>In addition to the above change, another new API is added which help the<o:p></o:p></pre> <pre>application get information about which queues have aged out flows after<o:p></o:p></pre> <pre>RTE_ETH_EVENT_FLOW_AGED event received. The queried queue id can be<o:p></o:p></pre> <pre>used in the above queue based query aged flows API.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>Signed-off-by: Xiaoyu Min <a href="mailto:jackmin@nvidia.com"><jackmin@nvidia.com></a><o:p></o:p></pre> <pre>---<o:p></o:p></pre> <pre> lib/ethdev/rte_flow.h | 82 ++++++++++++++++++++++++++++++++++++<o:p></o:p></pre> <pre> lib/ethdev/rte_flow_driver.h | 13 ++++++<o:p></o:p></pre> <pre> 2 files changed, 95 insertions(+)<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h<o:p></o:p></pre> <pre>index 38439fcd1d..a12becfe3b 100644<o:p></o:p></pre> <pre>--- a/lib/ethdev/rte_flow.h<o:p></o:p></pre> <pre>+++ b/lib/ethdev/rte_flow.h<o:p></o:p></pre> <pre>@@ -2810,6 +2810,7 @@ enum rte_flow_action_type {<o:p></o:p></pre> <pre> * See function rte_flow_get_aged_flows<o:p></o:p></pre> <pre> * see enum RTE_ETH_EVENT_FLOW_AGED<o:p></o:p></pre> <pre> * See struct rte_flow_query_age<o:p></o:p></pre> <pre>+ * See function rte_flow_get_q_aged_flows<o:p></o:p></pre> <pre> */<o:p></o:p></pre> <pre> RTE_FLOW_ACTION_TYPE_AGE,<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>@@ -5624,6 +5625,87 @@ rte_flow_async_action_handle_update(uint16_t port_id,<o:p></o:p></pre> <pre> const void *update,<o:p></o:p></pre> <pre> void *user_data,<o:p></o:p></pre> <pre> struct rte_flow_error *error);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+/**<o:p></o:p></pre> <pre>+ * @warning<o:p></o:p></pre> <pre>+ * @b EXPERIMENTAL: this API may change without prior notice.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * Get flow queues which have aged out flows on a given port.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * The application can use this function to query which queues have aged out flows after<o:p></o:p></pre> <pre>+ * a RTE_ETH_EVENT_FLOW_AGED event is received so the returned queue id can be used to<o:p></o:p></pre> <pre>+ * get aged out flows on this given queue by call rte_flow_get_q_aged_flows.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * This function can be called from the event callback or synchronously regardless of the event.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * @param port_id<o:p></o:p></pre> <pre>+ * Port identifier of Ethernet device.<o:p></o:p></pre> <pre>+ * @param[in, out] queue_id<o:p></o:p></pre> <pre>+ * Array of queue id that will be set.<o:p></o:p></pre> <pre>+ * @param[in] nb_queue_id<o:p></o:p></pre> <pre>+ * Maximum number of the queue id that can be returned.<o:p></o:p></pre> <pre>+ * This value should be equal to the size of the queue_id array.<o:p></o:p></pre> <pre>+ * @param[out] error<o:p></o:p></pre> <pre>+ * Perform verbose error reporting if not NULL. Initialized in case of<o:p></o:p></pre> <pre>+ * error only.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * @return<o:p></o:p></pre> <pre>+ * if nb_queue_id is 0, return the amount of all queues which have aged out flows.<o:p></o:p></pre> <pre>+ * if nb_queue_id is not 0 , return the amount of queues which have aged out flows<o:p></o:p></pre> <pre>+ * reported in the queue_id array, otherwise negative errno value.<o:p></o:p></pre> </blockquote> <pre><o:p> </o:p></pre> <pre>I'm sorry, but it is unclear for me what happens if provided array is<o:p></o:p></pre> <pre>insufficient to return all queues. IMHO, we still should provide as<o:p></o:p></pre> <pre>much as we can. The question is how to report that we have more queues.<o:p></o:p></pre> <pre>It looks like the only sensible way is to return value greater than<o:p></o:p></pre> <pre>nb_queue_id.<o:p></o:p></pre> <pre><o:p> </o:p></pre> </blockquote> <pre>I think that just like any other function, this function should return the max based on the requested number.<o:p></o:p></pre> <pre>Returning bigger number may result in out of buf issues, or require extra validation step from application.<o:p></o:p></pre> <pre>In addition as far as I can see the common practice in DPDK is to return the requested number.<o:p></o:p></pre> </blockquote> Yes, it just likes other functions. <o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre>I have other concern with this function, from my understanding this function will be called on the service thread<o:p></o:p></pre> <pre>that handels the aging event, after calling this function the application still needs to propagate the event to the<o:p></o:p></pre> <pre>correct threads.<o:p></o:p></pre> <pre>I think it will be better if the event itself will hold which queue triggered the aging. Or even better to get the<o:p></o:p></pre> </blockquote> As discussed in v1, there seems no good place in the current callback function to pass this kind of information from driver to application.<o:p></o:p> Or you have a better idea?<o:p></o:p> [Ori] Maybe use the new queues, for example maybe application can get the notification as part of the polling function. maybe it can even get the aged rules.<o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre>notification on the correct thread. (I know it is much more complicated but maybe it is worth the effort,<o:p></o:p></pre> <pre>since this can be used for other cases)<o:p></o:p></pre> </blockquote> Yes this will be better but I don't really know how to inform the correct thread. <o:p></o:p> Or we just let application register callback per flow queue?<o:p></o:p> Something like: rte_flow_queue_callback_register(), and PMD call rte_flow_queue_callback_process() to invoke<o:p></o:p> the callback functions registered to this queue only?<o:p></o:p> [Ori] that is a good suggestion or the one I suggested above. And we can also improve the current event, the question is what are the advantages of any approach and what are the downsides.<o:p></o:p> <o:p> </o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre><o:p> </o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>+ *<o:p></o:p></pre> <pre>+ * @see rte_flow_action_age<o:p></o:p></pre> <pre>+ * @see RTE_ETH_EVENT_FLOW_AGED<o:p></o:p></pre> <pre>+ */<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+__rte_experimental<o:p></o:p></pre> <pre>+int<o:p></o:p></pre> <pre>+rte_flow_get_aged_queues(uint16_t port_id, uint32_t queue_id[], uint32_t nb_queue_id,<o:p></o:p></pre> <pre>+ struct rte_flow_error *error);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+/**<o:p></o:p></pre> <pre>+ * @warning<o:p></o:p></pre> <pre>+ * @b EXPERIMENTAL: this API may change without prior notice.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * Get aged-out flows of a given port on the given flow queue.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * RTE_ETH_EVENT_FLOW_AGED event will be triggered at least one new aged out flow was<o:p></o:p></pre> <pre>+ * detected on any flow queue after the last call to rte_flow_get_q_aged_flows.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * The application can use rte_flow_get_aged_queues to query which queues have aged<o:p></o:p></pre> <pre>+ * out flows after RTE_ETH_EVEN_FLOW_AGED event.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * If application configure port attribute without RTE_FLOW_PORT_FLAG_STRICT_QUEUE<o:p></o:p></pre> <pre>+ * the @p queue_id will be ignored.<o:p></o:p></pre> <pre>+ * This function can be called to get the aged flows asynchronously from the<o:p></o:p></pre> <pre>+ * event callback or synchronously regardless the event.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * @param port_id<o:p></o:p></pre> <pre>+ * Port identifier of Ethernet device.<o:p></o:p></pre> <pre>+ * @param queue_id<o:p></o:p></pre> <pre>+ * Flow queue to query. Ignored when RTE_FLOW_PORT_FLAG_STRICT_QUEUE not set.<o:p></o:p></pre> <pre>+ * @param[in, out] contexts<o:p></o:p></pre> <pre>+ * The address of an array of pointers to the aged-out flows contexts.<o:p></o:p></pre> <pre>+ * @param[in] nb_contexts<o:p></o:p></pre> <pre>+ * The length of context array pointers.<o:p></o:p></pre> <pre>+ * @param[out] error<o:p></o:p></pre> <pre>+ * Perform verbose error reporting if not NULL. Initialized in case of<o:p></o:p></pre> <pre>+ * error only.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * @return<o:p></o:p></pre> <pre>+ * if nb_contexts is 0, return the amount of all aged contexts.<o:p></o:p></pre> <pre>+ * if nb_contexts is not 0 , return the amount of aged flows reported<o:p></o:p></pre> <pre>+ * in the context array, otherwise negative errno value.<o:p></o:p></pre> <pre>+ *<o:p></o:p></pre> <pre>+ * @see rte_flow_action_age<o:p></o:p></pre> <pre>+ * @see RTE_ETH_EVENT_FLOW_AGED<o:p></o:p></pre> <pre>+ * @see rte_flow_port_flag<o:p></o:p></pre> <pre>+ */<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+__rte_experimental<o:p></o:p></pre> <pre>+int<o:p></o:p></pre> <pre>+rte_flow_get_q_aged_flows(uint16_t port_id, uint32_t queue_id, void **contexts,<o:p></o:p></pre> <pre>+ uint32_t nb_contexts, struct rte_flow_error *error);<o:p></o:p></pre> <pre> #ifdef __cplusplus<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre> #endif<o:p></o:p></pre> <pre>diff --git a/lib/ethdev/rte_flow_driver.h b/lib/ethdev/rte_flow_driver.h<o:p></o:p></pre> <pre>index 2bff732d6a..b665170bf4 100644<o:p></o:p></pre> <pre>--- a/lib/ethdev/rte_flow_driver.h<o:p></o:p></pre> <pre>+++ b/lib/ethdev/rte_flow_driver.h<o:p></o:p></pre> <pre>@@ -260,6 +260,19 @@ struct rte_flow_ops {<o:p></o:p></pre> <pre> const void *update,<o:p></o:p></pre> <pre> void *user_data,<o:p></o:p></pre> <pre> struct rte_flow_error *error);<o:p></o:p></pre> <pre>+ /** See rte_flow_get_aged_queues() */<o:p></o:p></pre> <pre>+ int (*get_aged_queues)<o:p></o:p></pre> <pre>+ (uint16_t port_id,<o:p></o:p></pre> <pre>+ uint32_t queue_id[],<o:p></o:p></pre> <pre>+ uint32_t nb_queue_id,<o:p></o:p></pre> <pre>+ struct rte_flow_error *error);<o:p></o:p></pre> <pre>+ /** See rte_flow_get_q_aged_flows() */<o:p></o:p></pre> <pre>+ int (*get_q_aged_flows)<o:p></o:p></pre> <pre>+ (uint16_t port_id,<o:p></o:p></pre> <pre>+ uint32_t queue_id,<o:p></o:p></pre> <pre>+ void **contexts,<o:p></o:p></pre> <pre>+ uint32_t nb_contexts,<o:p></o:p></pre> <pre>+ struct rte_flow_error *error);<o:p></o:p></pre> <pre> };<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre> /**<o:p></o:p></pre> </blockquote> </blockquote> <pre><o:p> </o:p></pre> </blockquote> </div> </div> </body> </html>