Re: [dpdk-dev] [PATCH v2] app/testpmd: support flow aging

[Matan Azrad <matan@mellanox.com>]

From: Ferruh Yigit:
> 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 <dongz@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.

It will be good to the user to reduce logs of non-interested events (maybe even more often) which makes his log bigger.


> 
> >
> >
> >>>
> >>> 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.
> >