DPDK patches and discussions
 help / color / mirror / Atom feed
From: Matan Azrad <matan@mellanox.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	Bill Zhou <dongz@mellanox.com>,
	"wenzhuo.lu@intel.com" <wenzhuo.lu@intel.com>,
	"jingjing.wu@intel.com" <jingjing.wu@intel.com>,
	"bernard.iremonger@intel.com" <bernard.iremonger@intel.com>,
	Ori Kam <orika@mellanox.com>,
	"john.mcnamara@intel.com" <john.mcnamara@intel.com>,
	"marko.kovacevic@intel.com" <marko.kovacevic@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: support flow aging
Date: Fri, 1 May 2020 06:51:57 +0000
Message-ID: <AM0PR0502MB4019926D4E44B6341A76E063D2AB0@AM0PR0502MB4019.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <5fc8bc00-e4b1-b2c4-1caf-03dea0456f4c@intel.com>

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.

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.

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.

  reply	other threads:[~2020-05-01  6:52 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-24 10:55 [dpdk-dev] [PATCH] " Bill Zhou
2020-04-24 16:25 ` Ferruh Yigit
2020-04-26  7:23   ` Bill Zhou
2020-04-27 14:13     ` Ferruh Yigit
2020-04-27 15:12       ` Matan Azrad
2020-04-30 22:26         ` Ferruh Yigit
2020-04-30 15:53 ` [dpdk-dev] [PATCH v2] " Bill Zhou
2020-04-30 22:43   ` Ferruh Yigit
2020-05-01  6:51     ` Matan Azrad [this message]
2020-05-01  9:27       ` Ferruh Yigit
2020-05-01 11:28         ` Matan Azrad
2020-05-01 11:54           ` Ferruh Yigit
2020-05-01 12:45             ` Matan Azrad
2020-05-01 13:38               ` Ferruh Yigit
2020-05-01 15:14                 ` Matan Azrad
2020-05-01 15:44                   ` Ferruh Yigit
2020-05-02 14:00   ` [dpdk-dev] [PATCH v3] " Bill Zhou
2020-05-03  8:59     ` [dpdk-dev] [PATCH v4] " Bill Zhou
2020-05-03  9:46       ` Matan Azrad
2020-05-03 14:58       ` Ori Kam
2020-05-05  8:37       ` Ferruh Yigit
2020-05-05  9:11         ` Matan Azrad
2020-05-05  9:23           ` Ferruh Yigit
2020-05-05  9:49       ` [dpdk-dev] [PATCH v5] " Bill Zhou
2020-05-05 10:09         ` Ori Kam
2020-05-05 15:11           ` Ferruh Yigit
2020-05-06  8:04             ` Matan Azrad

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM0PR0502MB4019926D4E44B6341A76E063D2AB0@AM0PR0502MB4019.eurprd05.prod.outlook.com \
    --to=matan@mellanox.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=dongz@mellanox.com \
    --cc=ferruh.yigit@intel.com \
    --cc=jingjing.wu@intel.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=orika@mellanox.com \
    --cc=wenzhuo.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git