ethdev: add template table resize API

  Template table creation API sets table flows capacity.
If application needs more flows then the table was designed for,
the following procedures must be completed:
1. Create a new template table with larger flows capacity.
2. Re-create existing flows in the new table and delete flows from
   the original table.
3. Destroy original table.

Application cannot always execute that procedure:
* Port may not have sufficient resources to allocate a new table
  while maintaining original table.
* Application may not have existing flows "recipes" to re-create
  flows in a new table.

The patch defines a new API that allows application to resize
existing template table:

* Resizable template table must be created with the
RTE_FLOW_TABLE_SPECIALIZE_RESIZABLE_TABLE bit set.

* Application resizes existing table with the
  `rte_flow_template_table_resize()` function call.
  The table resize procedure updates the table maximal flow number
  only. Other table attributes are not affected by the table resize.
  ** The table resize procedure must not interrupt
     existing table flows operations in hardware.
  ** The table resize procedure must not alter flow handlers held by
     application.

* After `rte_flow_template_table_resize()` returned, application must
  update all existing table flow rules by calling
  `rte_flow_async_update_resized()`.
  The table resize procedure does not change application flow handler.
  However, flow object can reference internal PMD resources that are
  obsolete after table resize.
  `rte_flow_async_update_resized()` moves internal flow references
  to the updated table resources.
  The flow update must not interrupt hardware flow operations.

* When all table flow were updated, application must call
  `rte_flow_template_table_resize_complete()`.
  The function releases PMD resources related to the original
  table.
  Application can start new table resize after
  `rte_flow_template_table_resize_complete()` returned.

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>

---
 app/test-pmd/cmdline_flow.c            |  86 +++++++++++++++++++--
 app/test-pmd/config.c                  | 102 +++++++++++++++++++++++++
 app/test-pmd/testpmd.h                 |   6 ++
 doc/guides/rel_notes/release_24_03.rst |   2 +
 lib/ethdev/ethdev_trace.h              |  33 ++++++++
 lib/ethdev/ethdev_trace_points.c       |   9 +++
 lib/ethdev/rte_flow.c                  |  69 +++++++++++++++++
 lib/ethdev/rte_flow.h                  |  97 +++++++++++++++++++++++
 lib/ethdev/rte_flow_driver.h           |  15 ++++
 lib/ethdev/version.map                 |   3 +
 10 files changed, 417 insertions(+), 5 deletions(-)
  

On 12/17/2023 9:32 AM, Gregory Etelson wrote:
> Template table creation API sets table flows capacity.
> If application needs more flows then the table was designed for,
> the following procedures must be completed:
> 1. Create a new template table with larger flows capacity.
> 2. Re-create existing flows in the new table and delete flows from
>    the original table.
> 3. Destroy original table.
> 
> Application cannot always execute that procedure:
> * Port may not have sufficient resources to allocate a new table
>   while maintaining original table.
> * Application may not have existing flows "recipes" to re-create
>   flows in a new table.
> 
> The patch defines a new API that allows application to resize
> existing template table:
> 
> * Resizable template table must be created with the
> RTE_FLOW_TABLE_SPECIALIZE_RESIZABLE_TABLE bit set.
> 
> * Application resizes existing table with the
>   `rte_flow_template_table_resize()` function call.
>   The table resize procedure updates the table maximal flow number
>   only. Other table attributes are not affected by the table resize.
>   ** The table resize procedure must not interrupt
>      existing table flows operations in hardware.
>   ** The table resize procedure must not alter flow handlers held by
>      application.
> 
> * After `rte_flow_template_table_resize()` returned, application must
>   update all existing table flow rules by calling
>   `rte_flow_async_update_resized()`.
>   The table resize procedure does not change application flow handler.
>   However, flow object can reference internal PMD resources that are
>   obsolete after table resize.
>   `rte_flow_async_update_resized()` moves internal flow references
>   to the updated table resources.
>   The flow update must not interrupt hardware flow operations.
> 
> * When all table flow were updated, application must call
>   `rte_flow_template_table_resize_complete()`.
>   The function releases PMD resources related to the original
>   table.
>   Application can start new table resize after
>   `rte_flow_template_table_resize_complete()` returned.
> 

Hi Gregory, Ori,

Why we need three separate APIs,
 rte_flow_template_table_resize
 rte_flow_async_update_resized
 rte_flow_template_table_resize_complete

Why not 'rte_flow_template_table_resize()' update existing flows and
release resources related to the original tables automatically?
  

Hello Ferruh,

>
> Hi Gregory, Ori,
>
> Why we need three separate APIs,
> rte_flow_template_table_resize
> rte_flow_async_update_resized
> rte_flow_template_table_resize_complete
>
> Why not 'rte_flow_template_table_resize()' update existing flows and
> release resources related to the original tables automatically?
>


Template table resize API allows to add new flows immediately after 
rte_flow_template_table_resize completed.
A multi-threaded application can add new and update old flows simultaneously.

A single resize-and-update API would require to lock PMD for entire operation.
For application with 1e6 flows doubling a table would end up with 
considerable down time.

The rte_flow_template_table_resize_complete was added for PMDs that cannot 
differentiate flows created before and after table resize.

Regards,
Gregory
  

On 1/29/2024 3:08 PM, Etelson, Gregory wrote:
> Hello Ferruh,
> 
>>
>> Hi Gregory, Ori,
>>
>> Why we need three separate APIs,
>> rte_flow_template_table_resize
>> rte_flow_async_update_resized
>> rte_flow_template_table_resize_complete
>>
>> Why not 'rte_flow_template_table_resize()' update existing flows and
>> release resources related to the original tables automatically?
>>
> 
> 
> Template table resize API allows to add new flows immediately after
> rte_flow_template_table_resize completed.
> A multi-threaded application can add new and update old flows
> simultaneously.
> 
> A single resize-and-update API would require to lock PMD for entire
> operation.
> For application with 1e6 flows doubling a table would end up with
> considerable down time.
> 

If a multi-threaded application can add new and updated old
simultaneously, this should be done via monolithic API, like:
{
  lock
    resize
  unlock
  for each flow
    lock
    update
    unlock
}

Perhaps questions is, is there a usecase that user does the resize but
doesn't want to update the old flows?

> The rte_flow_template_table_resize_complete was added for PMDs that
> cannot differentiate flows created before and after table resize.
> 

Can you please elaborate this?

Does it mean old flows and new flows require different handling and PMD
doesn't know how to differentiate old and new flows?
If so how update() converts old flows, there must be a way for driver to
differentiate them for update() to work.

Also if resize_complete() NOT called at all, does this mean PMD can't
handle any flows anymore as it can't differentiate old and new ones?
  

Hello Ferruh,

>
> If a multi-threaded application can add new and updated old
> simultaneously, this should be done via monolithic API, like:
> {
>  lock
>    resize
>  unlock
>  for each flow
>    lock
>    update
>    unlock
> }
>

The flow template API was designed for performance.
Application that implements the flow template API expects high
flows insertions, updates, and removals rates.
Locks are necessary for the table resize API.

During the monolithic resize, application has no control over
PMD locks. Even if resize and each update operations are
relatively fast, application should expect table lock collisions
in rules insertions, deletions and updates for the entire 
resize-and-update-and-update operation.

With the separate resize API, lock collisions are expected
during the resize phase only.
After table resize completed, all flow operations will obtain
a lock without collision.
Also, application does not have to update all flows at once.
Updates can be executed in batches scheduled by application.
Another use case: application can increase a table,
offload all new flows and run updates while hardware handles
network traffic according to the new flows scheme.

> Perhaps questions is, is there a usecase that user does the resize but
> doesn't want to update the old flows?

Please see below.

>
>> The rte_flow_template_table_resize_complete was added for PMDs that
>> cannot differentiate flows created before and after table resize.
>>
>
> Can you please elaborate this?
>
> Does it mean old flows and new flows require different handling and PMD
> doesn't know how to differentiate old and new flows?
> If so how update() converts old flows, there must be a way for driver to
> differentiate them for update() to work.
>
> Also if resize_complete() NOT called at all, does this mean PMD can't
> handle any flows anymore as it can't differentiate old and new ones?
>

Table resize API do not have any effect on running flows.
PMD uses the same procedure to create flows before and after table resize.
All flows instantiated from the same type before and after table resize.

Flow update that follows table resize manages PMD flow object location.
In MLX5 PMD, flow update moves an object that references a flow
from old table to a new table.
After all flows were moved to a new table, PMD has no need for the old
table and it can be released.

Since flow update manages PMD memory only,
application can ignore the update operation
if it does not care about effective memory management.

PMD can release the old flow table after all flows it referenced were
moved to a new table only.
Event that notifies PMD about empty old table can be ether internal
or external.
Internal event assumes PMD ability to track flows in a table.
External event in form of application call is more general approach.

Application must call resize_complete after it moved all flows
to the new table. That call notifies PMD that it safe to release resources 
related to old table.
If application did not update flows it must not call resize_complte.

Application can create new flows after table resize
regardless if it managed PMD memory with update and
resize_complete calls or not.
  

On 1/30/2024 12:46 PM, Etelson, Gregory wrote:
> Hello Ferruh,
> 
>>
>> If a multi-threaded application can add new and updated old
>> simultaneously, this should be done via monolithic API, like:
>> {
>>  lock
>>    resize
>>  unlock
>>  for each flow
>>    lock
>>    update
>>    unlock
>> }
>>
> 
> The flow template API was designed for performance.
> Application that implements the flow template API expects high
> flows insertions, updates, and removals rates.
> Locks are necessary for the table resize API.
> 
> During the monolithic resize, application has no control over
> PMD locks. Even if resize and each update operations are
> relatively fast, application should expect table lock collisions
> in rules insertions, deletions and updates for the entire
> resize-and-update-and-update operation.
> 
> With the separate resize API, lock collisions are expected
> during the resize phase only.
> After table resize completed, all flow operations will obtain
> a lock without collision.
> Also, application does not have to update all flows at once.
> Updates can be executed in batches scheduled by application.
> Another use case: application can increase a table,
> offload all new flows and run updates while hardware handles
> network traffic according to the new flows scheme.
> 
>> Perhaps questions is, is there a usecase that user does the resize but
>> doesn't want to update the old flows?
> 
> Please see below.
> 
>>
>>> The rte_flow_template_table_resize_complete was added for PMDs that
>>> cannot differentiate flows created before and after table resize.
>>>
>>
>> Can you please elaborate this?
>>
>> Does it mean old flows and new flows require different handling and PMD
>> doesn't know how to differentiate old and new flows?
>> If so how update() converts old flows, there must be a way for driver to
>> differentiate them for update() to work.
>>
>> Also if resize_complete() NOT called at all, does this mean PMD can't
>> handle any flows anymore as it can't differentiate old and new ones?
>>
> 
> Table resize API do not have any effect on running flows.
> PMD uses the same procedure to create flows before and after table resize.
> All flows instantiated from the same type before and after table resize.
> 
> Flow update that follows table resize manages PMD flow object location.
> In MLX5 PMD, flow update moves an object that references a flow
> from old table to a new table.
> After all flows were moved to a new table, PMD has no need for the old
> table and it can be released.
> 
> Since flow update manages PMD memory only,
> application can ignore the update operation
> if it does not care about effective memory management.
> 
> PMD can release the old flow table after all flows it referenced were
> moved to a new table only.
> Event that notifies PMD about empty old table can be ether internal
> or external.
> Internal event assumes PMD ability to track flows in a table.
> External event in form of application call is more general approach.
> 
> Application must call resize_complete after it moved all flows
> to the new table. That call notifies PMD that it safe to release
> resources related to old table.
> If application did not update flows it must not call resize_complte.
> 
> Application can create new flows after table resize
> regardless if it managed PMD memory with update and
> resize_complete calls or not.
>

Thanks for clarification.

So, by design, driver will keep the old table when it is resized.
- Can this have a performance impact, like when rules
updated/removed/inserted driver will need to look more tables?
- Or can this cause additional capacity complexity, like total number of
rules will be sum of rules in all tables, but new rules only can be
added to latest table, so number of rules can be more than size of
latest table.
- Or user can add more flows after resize() and this may not leave
enough room to update old rules to new table, what is expected behavior
for this case?
- Or if user did not updated rules at all after resize(), after each
rule deletion driver won't need to check if old table emptied and needs
to be freed?
- Or can user call resize() API multiple times, causing driver to
maintain multiple tables? How much memory overhead this may bring?


'rte_flow_async_update_resized()' API is called per flow, won't this
force application to trace which flows are created in new table and
which are in old table, so pushing additional work to application.

Or what will happen if update() fails in the middle of update, should
user retry, should PMD restore back the moved rules?



I understood the logic behind the dividing responsibility to multiple
APIs, and it makes sense, but it brings above complexities, and more
work to application.
Can it be possible to have monolithic API but only resize() part of it
is blocking and update() part and later remove table part done
asynchronously?


I will also put more comment on the patch based on latest understanding.
  

On 12/17/2023 9:32 AM, Gregory Etelson wrote:
> Template table creation API sets table flows capacity.
> If application needs more flows then the table was designed for,
> the following procedures must be completed:
> 1. Create a new template table with larger flows capacity.
> 2. Re-create existing flows in the new table and delete flows from
>    the original table.
> 3. Destroy original table.
> 
> Application cannot always execute that procedure:
> * Port may not have sufficient resources to allocate a new table
>   while maintaining original table.
> * Application may not have existing flows "recipes" to re-create
>   flows in a new table.
> 
> The patch defines a new API that allows application to resize
> existing template table:
> 
> * Resizable template table must be created with the
> RTE_FLOW_TABLE_SPECIALIZE_RESIZABLE_TABLE bit set.
> 
> * Application resizes existing table with the
>   `rte_flow_template_table_resize()` function call.
>   The table resize procedure updates the table maximal flow number
>   only. Other table attributes are not affected by the table resize.
>   ** The table resize procedure must not interrupt
>      existing table flows operations in hardware.
>   ** The table resize procedure must not alter flow handlers held by
>      application.
> 
> * After `rte_flow_template_table_resize()` returned, application must
>   update all existing table flow rules by calling
>   `rte_flow_async_update_resized()`.
>   The table resize procedure does not change application flow handler.
>   However, flow object can reference internal PMD resources that are
>   obsolete after table resize.
>   `rte_flow_async_update_resized()` moves internal flow references
>   to the updated table resources.
>   The flow update must not interrupt hardware flow operations.
> 
> * When all table flow were updated, application must call
>   `rte_flow_template_table_resize_complete()`.
>   The function releases PMD resources related to the original
>   table.
>   Application can start new table resize after
>   `rte_flow_template_table_resize_complete()` returned.
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>
> 
> ---
>  app/test-pmd/cmdline_flow.c            |  86 +++++++++++++++++++--
>  app/test-pmd/config.c                  | 102 +++++++++++++++++++++++++
>  app/test-pmd/testpmd.h                 |   6 ++
>

It helps to document added/updated command in the commit log, and in the
documentation 'testpmd_funcs.rst'.
Including new attribute to create table etc...

Please check following commit as sample:
Commit 98ab16778daa ("app/testpmd: add random item support")


>  doc/guides/rel_notes/release_24_03.rst |   2 +
>  lib/ethdev/ethdev_trace.h              |  33 ++++++++
>  lib/ethdev/ethdev_trace_points.c       |   9 +++
>  lib/ethdev/rte_flow.c                  |  69 +++++++++++++++++
>  lib/ethdev/rte_flow.h                  |  97 +++++++++++++++++++++++
>  lib/ethdev/rte_flow_driver.h           |  15 ++++
>  lib/ethdev/version.map                 |   3 +
>  10 files changed, 417 insertions(+), 5 deletions(-)
> 

Just to double check if there will be driver implementation of the new
APIs in this release?

<...>

>  
> +static __rte_always_inline bool
> +rte_flow_table_resizable(const struct rte_flow_template_table_attr *tbl_attr)
> +{
> +	return (tbl_attr->specialize &
> +		RTE_FLOW_TABLE_SPECIALIZE_RESIZABLE_TABLE) != 0;
> +}
> +

Ahh, this is the 4th API added in this patch, missed before.

Why it is exposed to the application?
What is the expected usage for the function, it doesn't get 'port_id' as
parameter and directly works on the table_attribute, feels like API
should be for drivers?
Why it is inlined?

<...>

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Change template table flow rules capacity.
> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param table
> + *   Template table to modify.
> + * @param nb_rules
> + *   New flow rules capacity.
> + * @param error
> + *   Perform verbose error reporting if not NULL.
> + *   PMDs initialize this structure in case of error only.
> + *
> + * @return
> + *   - (0) if success.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-ENOTSUP) if underlying device does not support this functionality.
> + *   - (-EINVAL) if *table* cannot be resized or invalid *nb_rules*
>

What is invalid 'nb_rules', does it make sense to document it in above
@param for it. Like if 0 valid, or is there a max, or is it allowed to
shrink the size of table?


> + */
> +__rte_experimental
> +int
> +rte_flow_template_table_resize(uint16_t port_id,
> +			       struct rte_flow_template_table *table,
> +			       uint32_t nb_rules,
> +			       struct rte_flow_error *error);
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Following table resize, update flow resources in port.
> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param queue
> + *   Flow queue for async operation.
> + * @param attr
> + *   Async operation attributes.
> + * @param rule
> + *   Flow rule to update.
> + * @param user_data
> + *   The user data that will be returned on async completion event.
> + * @param error
> + *   Perform verbose error reporting if not NULL.
> + *   PMDs initialize this structure in case of error only.
> + *
> + * @return
> + *   - (0) if success.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-ENOTSUP) if underlying device does not support this functionality.
> + *   - (-EINVAL) if *rule* cannot be updated.
> + */
> +__rte_experimental
> +int
> +rte_flow_async_update_resized(uint16_t port_id, uint32_t queue,
> +			      const struct rte_flow_op_attr *attr,
> +			      struct rte_flow *rule, void *user_data,
> +			      struct rte_flow_error *error);
> +

Should API have 'table' or 'template' in name, if it is out of this
context, it is not intuitive that update_resized refers to template
table resize. It may be confused as if this is something related to the
rule itself resize.


> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Following table resize, notify port that all table flows were updated.
> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param table
> + *   Template table that undergoing resize operation.
> + * @param error
> + *   Perform verbose error reporting if not NULL.
> + *   PMDs initialize this structure in case of error only.
> + *
> + * @return
> + *   - (0) if success.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-ENOTSUP) if underlying device does not support this functionality.
> + *   - (-EINVAL) PMD cannot complete table resize.
> + */
> +__rte_experimental
> +int
> +rte_flow_template_table_resize_complete(uint16_t port_id,
> +					struct rte_flow_template_table *table,
> +					struct rte_flow_error *error);
>

Does it make sense to add a new error type to differentiate
unrecoverable error (I don't know if there is any) and an error that is
caused by not all rules updated?

<...>

>  
>  /**
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 5c4917c020..e37bab9b81 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -316,6 +316,9 @@ EXPERIMENTAL {
>  	rte_eth_recycle_rx_queue_info_get;
>  	rte_flow_group_set_miss_actions;
>  	rte_flow_calc_table_hash;
> +	rte_flow_template_table_resize;
> +	rte_flow_async_update_resized;
> +	rte_flow_template_table_resize_complete;
>  

New APIs should go below "# added in 24.03" comment
  

Hello Ferruh,

> So, by design, driver will keep the old table when it is resized.
> - Can this have a performance impact, like when rules
> updated/removed/inserted driver will need to look more tables?
> - Or can this cause additional capacity complexity, like total number of
> rules will be sum of rules in all tables, but new rules only can be
> added to latest table, so number of rules can be more than size of
> latest table.
> - Or user can add more flows after resize() and this may not leave
> enough room to update old rules to new table, what is expected behavior
> for this case?
> - Or if user did not updated rules at all after resize(), after each
> rule deletion driver won't need to check if old table emptied and needs
> to be freed?
> - Or can user call resize() API multiple times, causing driver to
> maintain multiple tables? How much memory overhead this may bring?

After "resize, update, complete" sequence table performance must be
the same as before resize.
If application skiped updates or resize completion, performance is undefined.
Driver must verify that total flows number does not exceed capacity set in 
table resize.

> 'rte_flow_async_update_resized()' API is called per flow, won't this
> force application to trace which flows are created in new table and
> which are in old table, so pushing additional work to application.

Application must trace what flows require update after table resize.
As the general rule, all flows that were created before table resize call has 
returned must be updated:

"old" flows |<-----------------resize------------>| "new" flows
   update                                              keep
                unknown flow location: update

----------------------------TIME-------------------------------------->

In MLX5 PMD, if update was called with a "new" flow, the call returns will 
success, without changing the flow.

>
> Or what will happen if update() fails in the middle of update, should
> user retry, should PMD restore back the moved rules?
>

If flow update call failed, it treated as failure during flow create, update or 
destroy.

>
> I understood the logic behind the dividing responsibility to multiple
> APIs, and it makes sense, but it brings above complexities, and more
> work to application.
> Can it be possible to have monolithic API but only resize() part of it
> is blocking and update() part and later remove table part done
> asynchronously?
>

Table resize and single flow update operations consume approximately the same 
time duration.
An update of a table with 1_000_000 flows will consume driver for too much time.
During that time application will not be able to create, destroy or update 
existing "old" flows.
Such operation must be coordinated with application.

A driver could provide a batch flows update, but I don’t see how it helps.
It's ether update one or update all and the latter does not scale.

>
> I will also put more comment on the patch based on latest understanding.
>
>
  

Hello Ferruh,

>> Template table creation API sets table flows capacity.
>> If application needs more flows then the table was designed for,
>> the following procedures must be completed:
>> 1. Create a new template table with larger flows capacity.
>> 2. Re-create existing flows in the new table and delete flows from
>>    the original table.
>> 3. Destroy original table.
>>
>> Application cannot always execute that procedure:
>> * Port may not have sufficient resources to allocate a new table
>>   while maintaining original table.
>> * Application may not have existing flows "recipes" to re-create
>>   flows in a new table.
>>
>> The patch defines a new API that allows application to resize
>> existing template table:
>>
>> * Resizable template table must be created with the
>> RTE_FLOW_TABLE_SPECIALIZE_RESIZABLE_TABLE bit set.
>>
>> * Application resizes existing table with the
>>   `rte_flow_template_table_resize()` function call.
>>   The table resize procedure updates the table maximal flow number
>>   only. Other table attributes are not affected by the table resize.
>>   ** The table resize procedure must not interrupt
>>      existing table flows operations in hardware.
>>   ** The table resize procedure must not alter flow handlers held by
>>      application.
>>
>> * After `rte_flow_template_table_resize()` returned, application must
>>   update all existing table flow rules by calling
>>   `rte_flow_async_update_resized()`.
>>   The table resize procedure does not change application flow handler.
>>   However, flow object can reference internal PMD resources that are
>>   obsolete after table resize.
>>   `rte_flow_async_update_resized()` moves internal flow references
>>   to the updated table resources.
>>   The flow update must not interrupt hardware flow operations.
>>
>> * When all table flow were updated, application must call
>>   `rte_flow_template_table_resize_complete()`.
>>   The function releases PMD resources related to the original
>>   table.
>>   Application can start new table resize after
>>   `rte_flow_template_table_resize_complete()` returned.
>>
>> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>> Acked-by: Ori Kam <orika@nvidia.com>
>>
>> ---
>>  app/test-pmd/cmdline_flow.c            |  86 +++++++++++++++++++--
>>  app/test-pmd/config.c                  | 102 +++++++++++++++++++++++++
>>  app/test-pmd/testpmd.h                 |   6 ++
>>
>
> It helps to document added/updated command in the commit log, and in the
> documentation 'testpmd_funcs.rst'.
> Including new attribute to create table etc...
>
> Please check following commit as sample:
> Commit 98ab16778daa ("app/testpmd: add random item support")
>

I'll post v2 with an update.

>
>>  doc/guides/rel_notes/release_24_03.rst |   2 +
>>  lib/ethdev/ethdev_trace.h              |  33 ++++++++
>>  lib/ethdev/ethdev_trace_points.c       |   9 +++
>>  lib/ethdev/rte_flow.c                  |  69 +++++++++++++++++
>>  lib/ethdev/rte_flow.h                  |  97 +++++++++++++++++++++++
>>  lib/ethdev/rte_flow_driver.h           |  15 ++++
>>  lib/ethdev/version.map                 |   3 +
>>  10 files changed, 417 insertions(+), 5 deletions(-)
>>
>
> Just to double check if there will be driver implementation of the new
> APIs in this release?
>

I'll post driver patches after DPDK API approval.

> <...>
>
>>
>> +static __rte_always_inline bool
>> +rte_flow_table_resizable(const struct rte_flow_template_table_attr *tbl_attr)
>> +{
>> +     return (tbl_attr->specialize &
>> +             RTE_FLOW_TABLE_SPECIALIZE_RESIZABLE_TABLE) != 0;
>> +}
>> +
>
> Ahh, this is the 4th API added in this patch, missed before.
>
> Why it is exposed to the application?

That API is for application and a driver.

> What is the expected usage for the function, it doesn't get 'port_id' as
> parameter and directly works on the table_attribute, feels like API

There is no usage for `port_id` in that query.

> should be for drivers?
> Why it is inlined?

I prefer inline functions to macros.

>
> <...>
>
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * Change template table flow rules capacity.
>> + *
>> + * @param port_id
>> + *   Port identifier of Ethernet device.
>> + * @param table
>> + *   Template table to modify.
>> + * @param nb_rules
>> + *   New flow rules capacity.
>> + * @param error
>> + *   Perform verbose error reporting if not NULL.
>> + *   PMDs initialize this structure in case of error only.
>> + *
>> + * @return
>> + *   - (0) if success.
>> + *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-ENOTSUP) if underlying device does not support this functionality.
>> + *   - (-EINVAL) if *table* cannot be resized or invalid *nb_rules*
>>
>
> What is invalid 'nb_rules', does it make sense to document it in above
> @param for it. Like if 0 valid, or is there a max, or is it allowed to
> shrink the size of table?

I'll post v2 with an update.

>
>
>> + */
>> +__rte_experimental
>> +int
>> +rte_flow_template_table_resize(uint16_t port_id,
>> +                            struct rte_flow_template_table *table,
>> +                            uint32_t nb_rules,
>> +                            struct rte_flow_error *error);
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * Following table resize, update flow resources in port.
>> + *
>> + * @param port_id
>> + *   Port identifier of Ethernet device.
>> + * @param queue
>> + *   Flow queue for async operation.
>> + * @param attr
>> + *   Async operation attributes.
>> + * @param rule
>> + *   Flow rule to update.
>> + * @param user_data
>> + *   The user data that will be returned on async completion event.
>> + * @param error
>> + *   Perform verbose error reporting if not NULL.
>> + *   PMDs initialize this structure in case of error only.
>> + *
>> + * @return
>> + *   - (0) if success.
>> + *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-ENOTSUP) if underlying device does not support this functionality.
>> + *   - (-EINVAL) if *rule* cannot be updated.
>> + */
>> +__rte_experimental
>> +int
>> +rte_flow_async_update_resized(uint16_t port_id, uint32_t queue,
>> +                           const struct rte_flow_op_attr *attr,
>> +                           struct rte_flow *rule, void *user_data,
>> +                           struct rte_flow_error *error);
>> +
>
> Should API have 'table' or 'template' in name, if it is out of this
> context, it is not intuitive that update_resized refers to template
> table resize. It may be confused as if this is something related to the
> rule itself resize.

That function call should have been called
`rte_flow_async_update_flow_after_table_resize`
The current version was selected after a long debate.
Please suggest an alternative.

>
>
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * Following table resize, notify port that all table flows were updated.
>> + *
>> + * @param port_id
>> + *   Port identifier of Ethernet device.
>> + * @param table
>> + *   Template table that undergoing resize operation.
>> + * @param error
>> + *   Perform verbose error reporting if not NULL.
>> + *   PMDs initialize this structure in case of error only.
>> + *
>> + * @return
>> + *   - (0) if success.
>> + *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-ENOTSUP) if underlying device does not support this functionality.
>> + *   - (-EINVAL) PMD cannot complete table resize.
>> + */
>> +__rte_experimental
>> +int
>> +rte_flow_template_table_resize_complete(uint16_t port_id,
>> +                                     struct rte_flow_template_table *table,
>> +                                     struct rte_flow_error *error);
>>
>
> Does it make sense to add a new error type to differentiate
> unrecoverable error (I don't know if there is any) and an error that is
> caused by not all rules updated?

That API should not return unrecoverable error.
When it called, all hardware related updates already completed.
No new resources required.

>
> <...>
>
>>
>>  /**
>> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
>> index 5c4917c020..e37bab9b81 100644
>> --- a/lib/ethdev/version.map
>> +++ b/lib/ethdev/version.map
>> @@ -316,6 +316,9 @@ EXPERIMENTAL {
>>       rte_eth_recycle_rx_queue_info_get;
>>       rte_flow_group_set_miss_actions;
>>       rte_flow_calc_table_hash;
>> +     rte_flow_template_table_resize;
>> +     rte_flow_async_update_resized;
>> +     rte_flow_template_table_resize_complete;
>>
>
> New APIs should go below "# added in 24.03" comment
>
I'll post an update in v2.
  

On 1/30/2024 6:15 PM, Etelson, Gregory wrote:
> Hello Ferruh,
> 
>> So, by design, driver will keep the old table when it is resized.
>> - Can this have a performance impact, like when rules
>> updated/removed/inserted driver will need to look more tables?
>> - Or can this cause additional capacity complexity, like total number of
>> rules will be sum of rules in all tables, but new rules only can be
>> added to latest table, so number of rules can be more than size of
>> latest table.
>> - Or user can add more flows after resize() and this may not leave
>> enough room to update old rules to new table, what is expected behavior
>> for this case?
>> - Or if user did not updated rules at all after resize(), after each
>> rule deletion driver won't need to check if old table emptied and needs
>> to be freed?
>> - Or can user call resize() API multiple times, causing driver to
>> maintain multiple tables? How much memory overhead this may bring?
> 
> After "resize, update, complete" sequence table performance must be
> the same as before resize.
> If application skiped updates or resize completion, performance is
> undefined.
>

According below update, it is expected some time will pass until user
updates flows, during this time there may be some performance impact,
does it make sense to mention from it in API documentation?


> Driver must verify that total flows number does not exceed capacity set
> in table resize.
> 

OK, this is more complexity to driver


Is multiple resize() call supported?

In the worst case, think about a case, for each rule application first
increase the size by one and adds that rule. Is this supported and
should there be a limit how many resize() can be done?


>> 'rte_flow_async_update_resized()' API is called per flow, won't this
>> force application to trace which flows are created in new table and
>> which are in old table, so pushing additional work to application.
> 
> Application must trace what flows require update after table resize.
> As the general rule, all flows that were created before table resize
> call has returned must be updated:
> 
> "old" flows |<-----------------resize------------>| "new" flows
>   update                                              keep
>                unknown flow location: update
> 
> ----------------------------TIME-------------------------------------->
> 
> In MLX5 PMD, if update was called with a "new" flow, the call returns
> will success, without changing the flow.
> 

At least this makes life easy for the application, can we document this
behavior as API requirement?

But still in a case there is high amount of flow insertion and deletion
happening in parallel, how application call the update(), it may still
need to keep list of flows to update.

>>
>> Or what will happen if update() fails in the middle of update, should
>> user retry, should PMD restore back the moved rules?
>>
> 
> If flow update call failed, it treated as failure during flow create,
> update or destroy.
> 
>>
>> I understood the logic behind the dividing responsibility to multiple
>> APIs, and it makes sense, but it brings above complexities, and more
>> work to application.
>> Can it be possible to have monolithic API but only resize() part of it
>> is blocking and update() part and later remove table part done
>> asynchronously?
>>
> 
> Table resize and single flow update operations consume approximately the
> same time duration.
>

Ah, thanks. I was missing this bit of information.


> An update of a table with 1_000_000 flows will consume driver for too
> much time.
> During that time application will not be able to create, destroy or
> update existing "old" flows.
> Such operation must be coordinated with application.
> 

Got it. Briefly I was trying to highlight the complexities that multiple
APIs bring, and responsibility pushed to the application,
but above performance impact explains the design decision.

Eventually application needs to get this hit, how application expected
to use these APIs?
First call resize(), after this point how application should handle
updating flows?


Can something like async updating flow work? Like driver returns success
but it sets a thread that in background moves flows in a loop, if it
gets the lock and release it per flow, this lets other thread get the
lock for other flow related operations?

> A driver could provide a batch flows update, but I don’t see how it helps.
> It's ether update one or update all and the latter does not scale.
> 
>>
>> I will also put more comment on the patch based on latest understanding.
>>
>>
  

Hello Ferruh,

I work on the patch update with detailed user guide for
the table resize API.

Regards,
Gregory


>>> So, by design, driver will keep the old table when it is resized.
>>> - Can this have a performance impact, like when rules
>>> updated/removed/inserted driver will need to look more tables?
>>> - Or can this cause additional capacity complexity, like total number of
>>> rules will be sum of rules in all tables, but new rules only can be
>>> added to latest table, so number of rules can be more than size of
>>> latest table.
>>> - Or user can add more flows after resize() and this may not leave
>>> enough room to update old rules to new table, what is expected behavior
>>> for this case?
>>> - Or if user did not updated rules at all after resize(), after each
>>> rule deletion driver won't need to check if old table emptied and needs
>>> to be freed?
>>> - Or can user call resize() API multiple times, causing driver to
>>> maintain multiple tables? How much memory overhead this may bring?
>>
>> After "resize, update, complete" sequence table performance must be
>> the same as before resize.
>> If application skiped updates or resize completion, performance is
>> undefined.
>>
>
> According below update, it is expected some time will pass until user
> updates flows, during this time there may be some performance impact,
> does it make sense to mention from it in API documentation?
>
>
>> Driver must verify that total flows number does not exceed capacity set
>> in table resize.
>>
>
> OK, this is more complexity to driver
>
>
> Is multiple resize() call supported?
>
> In the worst case, think about a case, for each rule application first
> increase the size by one and adds that rule. Is this supported and
> should there be a limit how many resize() can be done?
>
>
>>> 'rte_flow_async_update_resized()' API is called per flow, won't this
>>> force application to trace which flows are created in new table and
>>> which are in old table, so pushing additional work to application.
>>
>> Application must trace what flows require update after table resize.
>> As the general rule, all flows that were created before table resize
>> call has returned must be updated:
>>
>> "old" flows |<-----------------resize------------>| "new" flows
>>   update                                              keep
>>                unknown flow location: update
>>
>> ----------------------------TIME-------------------------------------->
>>
>> In MLX5 PMD, if update was called with a "new" flow, the call returns
>> will success, without changing the flow.
>>
>
> At least this makes life easy for the application, can we document this
> behavior as API requirement?
>
> But still in a case there is high amount of flow insertion and deletion
> happening in parallel, how application call the update(), it may still
> need to keep list of flows to update.
>
>>>
>>> Or what will happen if update() fails in the middle of update, should
>>> user retry, should PMD restore back the moved rules?
>>>
>>
>> If flow update call failed, it treated as failure during flow create,
>> update or destroy.
>>
>>>
>>> I understood the logic behind the dividing responsibility to multiple
>>> APIs, and it makes sense, but it brings above complexities, and more
>>> work to application.
>>> Can it be possible to have monolithic API but only resize() part of it
>>> is blocking and update() part and later remove table part done
>>> asynchronously?
>>>
>>
>> Table resize and single flow update operations consume approximately the
>> same time duration.
>>
>
> Ah, thanks. I was missing this bit of information.
>
>
>> An update of a table with 1_000_000 flows will consume driver for too
>> much time.
>> During that time application will not be able to create, destroy or
>> update existing "old" flows.
>> Such operation must be coordinated with application.
>>
>
> Got it. Briefly I was trying to highlight the complexities that multiple
> APIs bring, and responsibility pushed to the application,
> but above performance impact explains the design decision.
>
> Eventually application needs to get this hit, how application expected
> to use these APIs?
> First call resize(), after this point how application should handle
> updating flows?
>
>
> Can something like async updating flow work? Like driver returns success
> but it sets a thread that in background moves flows in a loop, if it
> gets the lock and release it per flow, this lets other thread get the
> lock for other flow related operations?
>
>> A driver could provide a batch flows update, but I don’t see how it helps.
>> It's ether update one or update all and the latter does not scale.
>>
>>>
>>> I will also put more comment on the patch based on latest understanding.
>>>
>>>
>
>