> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, January 30, 2020 11:13 AM
> To: Lukas Bartosik <lbarto...@marvell.com>; Anoob Joseph
> <ano...@marvell.com>; Akhil Goyal <akhil.go...@nxp.com>; Nicolau, Radu
> <radu.nico...@intel.com>; Thomas Monjalon <tho...@monjalon.net>
> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Narayana Prasad Raju
> Athreya <pathr...@marvell.com>; Ankur Dwivedi
> <adwiv...@marvell.com>; Archana Muniganti <march...@marvell.com>; Tejasree
> Kondoj <ktejas...@marvell.com>; Vamsi Krishna
> Attunuru <vattun...@marvell.com>; dev@dpdk.org
> Subject: RE: [EXT] RE: [PATCH v2 09/12] examples/ipsec-secgw: add eventmode
> to ipsec-secgw
>
> Hi Lukasz,
>
> > >>
> > >> /*
> > >> * 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.
> >
> > > 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.
> >
> > > 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.
