Hi Konstantin,

On 31.01.2020 02:09, Lukasz Bartosik wrote:

Hi Konstantin,

On 30.01.2020 23:21, Ananyev, Konstantin wrote:

quoted

quoted

-----Original Message-----
From: Ananyev, Konstantin
Sent: Thursday, January 30, 2020 11:13 AM
To: Lukas Bartosik <redacted>; Anoob Joseph <redacted>; Akhil Goyal <redacted>; Nicolau, Radu
[off-list ref]; Thomas Monjalon [off-list ref]
Cc: Jerin Jacob Kollanukkaran <redacted>; Narayana Prasad Raju Athreya <redacted>; Ankur Dwivedi
[off-list ref]; Archana Muniganti [off-list ref]; Tejasree Kondoj [off-list ref]; Vamsi Krishna
Attunuru [off-list ref]; dev@dpdk.org
Subject: RE: [EXT] RE: [PATCH v2 09/12] examples/ipsec-secgw: add eventmode to ipsec-secgw

Hi Lukasz,

quoted

quoted

quoted

 /*
  * RX/TX HW offload capabilities to enable/use on ethernet ports.

@@ -1185,7 +1205,7 @@ main_loop(__attribute__((unused)) void *dummy)
 }

 static int32_t
-check_params(void)
+check_params(struct eh_conf *eh_conf)
 {
 	uint8_t lcore;
 	uint16_t portid;

@@ -1220,6 +1240,14 @@ check_params(void)
 			return -1;
 		}
 	}
+
+	if (eh_conf->mode == EH_PKT_TRANSFER_MODE_POLL) {
+		if (schedule_type) {
+			printf("error: option --schedule-type applies only to event mode\n");
+			return -1;
+		}
+	}

As a nit - might be better to keep check_params() intact,
and put this new check above into a separate function?
check_eh_conf() or so?

[Lukasz] I will put the check into new check_eh_conf() function.

quoted

Another thing it seems a bit clumsy construction to have global var (scheduler_type)
just to figure out was particular option present on command line or not.
Probably simler way to avoid it - set initially em_conf->ext_params.sched_type to
some invalid value (-1 or so). Then after parse args you can check did its value
change or not.

[Lukasz] I will change it in V3.

quoted

As alternative thought: wouldn't it be better to unite both  --transfer-mode
and --schedule-type options into one?
Then possible values for this unite option would be:
"poll"
"event" (expands to "event-ordered")
"event-ordered"
"event-atomic"
"event-parallel"
And this situation you are checking above simply wouldn't be possible.
Again probably would be easier/simpler for users.

[Lukasz] I would rather not combine event mode parameters into one for two reason:
- to be consistent with poll where one configuration item is controlled with one option,
- if we come up with a need to add a new event mode parameter in future then we
we will need to split event-ordered back to --transfer-mode and --schedule-type
to be consistent with how with provide event mode command line options.

I thought for future mods we can just keep adding new types here:
"event-xxx", "poll-yyy", etc.
But if you think separate ones is a better approach - I am fine.

Probably one extra suggestion - would it make sense to change name for
that option to have 'event' inside?
'--event-scheduler' or so.
Will probably make things a bit more clear.

[Lukasz] I will rename option --schedule-type to --event-scheduler

[Lukasz] After reconsideration my proposal is to change option --schedule-type to --event-schedule-type. Are you ok with that ?