Hi Konstantin,
On 31.01.2020 02:09, Lukasz Bartosik wrote:
> 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
>
[Lukasz] After reconsideration my proposal is to change option --schedule-type
to --event-schedule-type. Are you ok with that ?
