Hi Konstantin,
On 30.01.2020 23:21, Ananyev, Konstantin wrote:
>
>
>> -----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.
[Lukasz] I will rename option --schedule-type to --event-scheduler
