On 5/1/2020 7:51 AM, Matan Azrad wrote:
> Hi Ferruh
>
> From: Ferruh Yigit
>> On 4/30/2020 4:53 PM, Bill Zhou wrote:
>>> Currently, there is no way to check the aging event or to get the
>>> current aged flows in testpmd, this patch include those implements, it's
>> included:
>>> - Registering aging event when the testpmd application start, add new
>>> command to control if the event expose to the applications. If it's be
>>> set, when new flow be checked age out, there will be one-line output log.
>>> - Add new command to list all aged flows, meanwhile, we can set
>> parameter
>>> to destroy it.
>>>
>>> Signed-off-by: Bill Zhou <do...@mellanox.com>
>>> ---
>>> v2: Update the way of registering aging event, add new command to
>>> control if the event need be print or not. Update the output of the
>>> delete aged flow command format.
>>
>> <...>
>>
>>> @@ -19387,6 +19394,44 @@ cmdline_parse_inst_t cmd_showport_macs =
>> {
>>> },
>>> };
>>>
>>> +/* Enable/Disable flow aging event log */ struct
>>> +cmd_set_aged_flow_event_log_result {
>>> + cmdline_fixed_string_t set;
>>> + cmdline_fixed_string_t keyword;
>>> + cmdline_fixed_string_t enable;
>>> +};
>>> +cmdline_parse_token_string_t cmd_set_aged_flow_event_log_set =
>>> + TOKEN_STRING_INITIALIZER(struct
>> cmd_set_aged_flow_event_log_result,
>>> + set, "set");
>>> +cmdline_parse_token_string_t cmd_set_aged_flow_event_log_keyword
>> =
>>> + TOKEN_STRING_INITIALIZER(struct
>> cmd_set_aged_flow_event_log_result,
>>> + keyword, "aged_flow_event_log");
>>> +cmdline_parse_token_string_t cmd_set_aged_flow_event_log_enable =
>>> + TOKEN_STRING_INITIALIZER(struct
>> cmd_set_aged_flow_event_log_result,
>>> + enable, "on#off");
>>> +
>>> +static void
>>> +cmd_set_aged_flow_event_log_parsed(void *parsed_result,
>>> + __rte_unused struct cmdline *cl,
>>> + __rte_unused void *data)
>>> +{
>>> + struct cmd_set_aged_flow_event_log_result *res = parsed_result;
>>> + int enable = (strcmp(res->enable, "on") == 0) ? 1 : 0;
>>> + update_aging_event_log_status(enable);
>>> +}
>>> +
>>> +cmdline_parse_inst_t cmd_set_aged_flow_event_log = {
>>> + .f = cmd_set_aged_flow_event_log_parsed,
>>> + .data = NULL,
>>> + .help_str = "set aged_flow_event_log on|off",
>>
>> What do you think "set aged_flow_verbose on|off" to be more similar to
>> existing verbose command?
>
> Please see my comments below regard it.
>
>> <...>
>>
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -3068,6 +3068,21 @@ rmv_port_callback(void *arg)
>>> start_packet_forwarding(0);
>>> }
>>>
>>> +static int aged_flow_event_enable;
>>
>> Other global config values are at the beginning of the file, with a comment
>> on
>> them, can you do same with variable?
>
> +1
>
>>> +void update_aging_event_log_status(int enable) {
>>> + aged_flow_event_enable = enable;
>>> +}
>>> +
>>> +int aging_event_output(uint16_t portid)
>>
>> This can be a 'static' function.
>
> +1
>
>>> +{
>>> + if (aged_flow_event_enable) {
>>> + printf("port %u RTE_ETH_EVENT_FLOW_AGED triggered\n",
>> portid);
>>> + fflush(stdout);
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> /* This function is used by the interrupt thread */ static int
>>> eth_event_callback(portid_t port_id, enum rte_eth_event_type type,
>>> void *param, @@ -3098,6 +3113,8 @@ eth_event_callback(portid_t
>> port_id, enum rte_eth_event_type type, void *param,
>>> rmv_port_callback, (void
>> *)(intptr_t)port_id))
>>> fprintf(stderr, "Could not set up deferred device
>> removal\n");
>>> break;
>>> + case RTE_ETH_EVENT_FLOW_AGED:
>>> + aging_event_output(port_id);
>>
>> Can't this provide more information than port_id, like flow id etc, what
>> event_process function provides? can we print it too?
>> And what is the intended usage in real application, when flow aged event
>> occurred, should application destroy the flow? So it should know the flow,
>> right?
>
> Probably Yes Ferruh, I will add details, maybe it will be clearer:
>
> As described well in rte_flow_get_aged_flows API description, there are 2
> suggested options for the application:
>
> 1. To call rte_flow_get_aged_flows from the AGED event callback in order to
> get the aged flow contexts of the triggered port.
> 2. To call rte_flow_get_aged_flows in any time application wants.
I see, for the testpmd implementation what do you think getting the aged flow
list and print them in event callback, this can be good as sample of the
intended usage?
>
> It is probably depends in the way the application wants to synchronize flow
> APIs calls.
>
> The application just gets the information of the aged-out flows context from
> the above API and can do any flow operation for it, and yes, the most
> expected case is to destroy the flows.
>
> Bill added 2 testpmd commands:
>
> 1. To use rte_flow_get_aged_flows and to print a list of all the aged-out
> flows (with option to destroy them directly by the command).
> 2. A Boolean to indicate the application whether to notify the testpmd user
> about new aged-out flows(by print).
>
> By this way, the testpmd user can use the 2 approaches with minimum testpmd
> flow management change.
>
> So, the Boolean var is more like "trigger testpmd user notification", not
> like regular verbose options.
As you already know, event always registered and callback always called, this
command only defines to print a log in the callback or not, so I believe
'verbose' suits here,
main concern is there are many testpmd commands and it is hard to remember them
(usage is not intuitive, I had need to check source code to find a command many
times), trying to make them as consistent as possible to help usage.
But I think as see your concern, "set aged_flow_verbose on|off" command can be
confused as if changing "flow aged #" command verbosity level.
What do you think about a more generic "set event_verbose on|off" command, which
can control to logging for all event handlers, but right now only for aged
events?
>
> Matan
>
>
>
>
>>
>> <...>
>>
>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>> @@ -1062,6 +1062,21 @@ Where:
>>>
>>> Check the NIC Datasheet for hardware limits.
>>>
>>> +aged flow event log set
>>> +~~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +Currently, the flow aging event is registered when the testpmd
>>> +application start, if new flow be checked age out, there will be one
>>> +output log for it. But some applications do not interest this event, use
>>> this
>> command can set if the event expose to the applications::
>>> +
>>> + testpmd> set aged_flow_event_log (on|off)
>>> +
>>> +For examine::
>>> +
>>> + testpmd> set aged_flow_event_log on
>>> + testpmd> port 0 RTE_ETH_EVENT_FLOW_AGED triggered
>>> + testpmd> set aged_flow_event_log off
>>> +
>>
>> This can be moved below the "set verbose" command since they are in
>> similar group.
