On 5/1/2020 12:28 PM, Matan Azrad wrote:
>
> Hi Ferruh
>
> From: Ferruh Yigit:
>> 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?
>
> Yes, we thought on it and I understand you,
> The issue with it is that we need to synchronize all the flow management in
> Testpmd and to protect any flow operation with a lock.
> Because the callback is probably coming from the host thread while other flow
> operations from different Testpmd thread(command line thread).
>
> It will do the patch very intrusive.
>
> The current approach(like the second suggestion) moves the synchronization to
> the testpmd user to access flows only from the command line thread while
> hinting to the user when a port holds some aged-out flows.
> I think it is better to stay the implementation simple.
OK
>
>>>
>>> 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?
>
> Looks good to me, but maybe instead of on | off it is better to use bitmap in
> order to indicate the event?
No objection but not sure that level fine grain needed now, or later, why not
add the basic on/off now and add the bitmap when we need to control for each
event.
>
>
>>>
>>> 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.
>
