[dpdk-dev] [PATCH v3 2/2] app/testpmd: add API for configuration of queue region

[Ferruh Yigit]

On 9/25/2017 10:25 AM, Zhao1, Wei wrote:
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, September 20, 2017 6:46 PM
>> To: Zhao1, Wei <wei.zhao1 at intel.com>; dev at dpdk.org; Wu, Jingjing
>> <jingjing.wu at intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: add API for
>> configuration of queue region
>>
>> On 9/15/2017 4:13 AM, Wei Zhao wrote:
>>> This patch add a API configuration of queue region in rss.
>>> It can parse the parameters of region index, queue number, queue start
>>> index, user priority, traffic classes and so on.
>>> According to commands from command line, it will call i40e private API
>>> and start the process of set or flush queue region configure. As this
>>> feature is specific for i40e, so private API will be used.
>>>
>>> Signed-off-by: Wei Zhao <wei.zhao1 at intel.com>
>>> ---
>>>  app/test-pmd/cmdline.c | 328
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>
>> Testpmd documentation also needs to be updated.
> 
> Do you mean the following doc or others?
> dpdk\doc\guides\testpmd_app_ug.rst

Yes this one, thanks.

> 
> 
>>
>>>  1 file changed, 328 insertions(+)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>> 0144191..060fcb1 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -637,6 +637,21 @@ static void cmd_help_long_parsed(void
>> *parsed_result,
>>>  			"ptype mapping update (port_id) (hw_ptype)
>> (sw_ptype)\n"
>>>  			"    Update a ptype mapping item on a port\n\n"
>>>
>>> +			"queue-region set port (port_id) region_id (value) "
>>> +			"queue_start_index (value) queue_num (value)\n"
>>> +			"    Set a queue region on a port\n\n"
>>> +
>>> +			"queue-region set (pf|vf) port (port_id) region_id
>> (value) "
>>> +			"flowtype (value)\n"
>>> +			"    Set a flowtype region index on a port\n\n"
>>> +
>>> +			"queue-region set port (port_id) UP (value)
>> region_id (value)\n"
>>> +			"    Set the mapping of User Priority to "
>>> +			"queue region on a port\n\n"
>>> +
>>> +			"queue-region flush (on|off) port (port_id)\n"
>>> +			"    flush all queue region related configuration\n\n"
>>
>> I keep doing same comment but I will do it again...
>>
>> Each patch adding a new feature looking from its own context and adding a
>> new root level command and this is making overall testpmd confusing.
>>
>> Since this is to set an option of the port, what do you think making this
>> command part of existing commands, like:
>> "port config #P queue-region ...."
>> OR
>> "set port #P queue-region ..." ?
> 
> What you said is very meaningful, but other feature liake ptype mapping use the same mode  and so on.
> maybe we should do a whole work to make CLI command style consistent.

Yes ptype does it, is seems it is one of the missed ones. Although we
can do a whole work for CLI commands, meanwhile I think new ones can be
added properly.

This may be good opportunity to remember broken window theory [1] :)

[1]
https://blog.codinghorror.com/the-broken-window-theory/

<...>