DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Harman Kalra <hkalra@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>,
	Thomas Monjalon <thomas@monjalon.net>,
	Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
	Ajit Khaparde <ajit.khaparde@broadcom.com>,
	Somnath Kotur <somnath.kotur@broadcom.com>,
	John Daley <johndale@cisco.com>,
	Hyong Youb Kim <hyonkim@cisco.com>,
	Yuying Zhang <Yuying.Zhang@intel.com>,
	Beilei Xing <beilei.xing@intel.com>,
	Qiming Yang <qiming.yang@intel.com>,
	Qi Zhang <qi.z.zhang@intel.com>, Wenjun Wu <wenjun1.wu@intel.com>,
	Dariusz Sosnowski <dsosnowski@nvidia.com>,
	Viacheslav Ovsiienko <viacheslavo@nvidia.com>,
	Ori Kam <orika@nvidia.com>, Suanming Mou <suanmingm@nvidia.com>,
	Matan Azrad <matan@nvidia.com>,
	Chaoyong He <chaoyong.he@corigine.com>
Subject: Re: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string
Date: Tue, 30 Jan 2024 23:09:44 +0000	[thread overview]
Message-ID: <9eddad36-4692-4439-aecf-731994bc2c7d@amd.com> (raw)
In-Reply-To: <BN9PR18MB4204E50681C799B8B30E502FC57E2@BN9PR18MB4204.namprd18.prod.outlook.com>

On 1/29/2024 6:20 PM, Harman Kalra wrote:
> Hi Ferruh
> 
> Thanks for the review
> Please find response inline
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Friday, January 26, 2024 7:13 PM
>> To: Harman Kalra <hkalra@marvell.com>; dev@dpdk.org; Thomas Monjalon
>> <thomas@monjalon.net>; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
>> <ajit.khaparde@broadcom.com>; Somnath Kotur
>> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
>> Hyong Youb Kim <hyonkim@cisco.com>; Yuying Zhang
>> <Yuying.Zhang@intel.com>; Beilei Xing <beilei.xing@intel.com>; Qiming Yang
>> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Wenjun Wu
>> <wenjun1.wu@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
>> Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Ori Kam
>> <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
>> Azrad <matan@nvidia.com>; Chaoyong He <chaoyong.he@corigine.com>
>> Subject: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor
>> devargs string
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 1/21/2024 7:19 PM, Harman Kalra wrote:
>>> Adding support for parsing multiple representor devargs strings passed
>>> to a PCI BDF. There may be scenario where port representors for
>>> various PFs or VFs under PFs are required and all these are
>>> representor ports shall be backed by single pci device. In such case
>>> port representors can be created using devargs string:
>>> <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
>>>
>>
>> This patch is to be able to parse multiple representor device argument, but I
>> am concerned how it is used.
> 
> In Marvell CNXK port representor solution all representors are backed by a single 
> PCI device (we call it as eswitch device). Eswitch device is not DPDK ethdev device
> but an internal device with NIC capabilities and is configured with as many no of
> Rxqs and Txqs as port representor required.
> Hence each port representor will have dedicated RxQ/TxQ pair to communicate with
> respective represented PF or VF. 
> 

ack, thanks for clarification.

Just to double check, is the cnxk driver support of new syntax planned
for this release?
It helps if the RFC is out before merging this patch.

> 
>>
>> When there are multiple representor ports backed up by same device, can't
>> it cause syncronization issues, like if two representor interfaces used for
>> conflicting configurations. Or if datapath will be supported, what if two
>> representator used simultaneously.
> 
> As I mentioned above, each port representor will have dedicated RxQ and TxQ,
> worker cores can poll on respective queues without any synchronization issue.
> I hope I am able to explain the use case.
> 

ack

>>
>>
> 
> 
> 
> 
>>
>> Meanwhile please check some comments below related to the parser code.
>>
>> <...>
>>
>>> @@ -459,9 +460,26 @@ eth_dev_devargs_tokenise(struct rte_kvargs
>> *arglist, const char *str_in)
>>>  			break;
>>>
>>>  		case 3: /* Parsing list */
>>> -			if (*letter == ']')
>>> -				state = 2;
>>> -			else if (*letter == '\0')
>>> +			if (*letter == ']') {
>>> +				/* For devargs having singles lists move to
>> state 2 once letter
>>> +				 * becomes ']' so each can be considered as
>> different pair key
>>> +				 * value. But in nested lists case e.g. multiple
>> representors
>>> +				 * case i.e. [pf[0-3],pfvf[3,4-6]], complete
>> nested list should
>>> +				 * be considered as one pair value, hence
>> checking if end of outer
>>> +				 * list ']' is reached else stay on state 3.
>>> +				 */
>>> +				if ((strcmp("representor", pair->key) == 0)
>> 	    &&
>>> +				    (*(letter + 1) != '\0' && *(letter + 2) != '\0'
>> &&
>>> +				     *(letter + 3) != '\0')			    &&
>>> +				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')
>> ||
>>> +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')
>> ||
>>> +				     (*(letter + 2) == 's' && *(letter + 3) == 'f')
>> ||
>>> +				     (*(letter + 2) == 'c' && isdigit(*(letter + 3)))
>> ||
>>> +				     (*(letter + 2) == '[' && isdigit(*(letter +
>> 3)))))
>>>
>>
>> Above is hard to understand but there are some assumptions in the input,
>> can we list supported syntax in the comment to make it more clear.
>>
>> For example following seems not support, can you please check if
>> intentional:
>> [vf0,vf1] // I am aware this can be put as vf[0,1] too [vf[0,1],3] [vf[0],vf[1]]
> 
> Intention behind above change is just to detect if its nested list i.e. multiple
> devargs (constituting lists as well) or a single devarg with a list.
> 
> pf0vf[1-4] is a single list devarg example, while [pf[0-3],pfvf[3,4-6]] is
> nested list example, where multiple devargs pf[0-3] and pfvf[3,4-6]
> (which are also lists) are bind to a list of devargs.
> 
> And as I mentioned in the comment, complete "nested list should be considered 
> as one pair value", hence entire list i.e. [vf[0,1],3] [vf[0],vf[1]] will be one
> value of arglist and later eth_dev_tokenise_representor_list() will tokenize
> and feed to rte_eth_devargs_parse_representor_ports().
> 
> Whether any format is supported or not should be handled by 
> rte_eth_devargs_parse_representor_ports()
> 

Agree but this is something user facing, user should know the syntax to
use it but some corner cases are not documented well and not trivial to
grasp from above code.

What do you think about adding some unit tests for parser, it can be
some pointer to the intention of what should be supported and what not,
also increases our confidence to the code?

> 
>>
>> I am not saying above should be supported, but syntax should be clear what
>> is supported what is not.
>>
>>
>> Also I can't check but is the redundant input verified, like:
>> [vf[0-3],vf[3,4]]
> 
> IMO, this should be handled by PMD, if any representor already created should
> return an error.
> 

fair enough


  reply	other threads:[~2024-01-30 23:09 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-11  6:44 [PATCH 0/2] multiple representors in one device Harman Kalra
2024-01-11  6:44 ` [PATCH 1/2] ethdev: parsing multiple representor devargs string Harman Kalra
2024-01-12  7:25   ` Andrew Rybchenko
2024-01-12  9:37     ` [EXT] " Harman Kalra
2024-01-12 12:42   ` David Marchand
2024-01-15 16:01     ` [EXT] " Harman Kalra
2024-01-11  6:44 ` [PATCH 2/2] doc: multiple representors in one device Harman Kalra
2024-01-12  7:26   ` Andrew Rybchenko
2024-01-15 16:01     ` [EXT] " Harman Kalra
2024-01-15 15:57 ` [PATCH v2 0/1] " Harman Kalra
2024-01-15 15:57   ` [PATCH v2 1/1] ethdev: parsing multiple representor devargs string Harman Kalra
2024-01-16  9:55 ` [PATCH v3 0/1] multiple representors in one device Harman Kalra
2024-01-16  9:55   ` [PATCH v3 1/1] ethdev: parsing multiple representor devargs string Harman Kalra
2024-01-17  8:47     ` Andrew Rybchenko
2024-01-21 19:30       ` [EXT] " Harman Kalra
2024-01-21 19:19 ` [PATCH v4 0/1] multiple representors in one device Harman Kalra
2024-01-21 19:19   ` [PATCH v4 1/1] ethdev: parsing multiple representor devargs string Harman Kalra
2024-01-22  1:13     ` Chaoyong He
2024-01-22  9:07       ` Harman Kalra
2024-01-22 10:10         ` Chaoyong He
2024-01-25  5:28     ` Harman Kalra
2024-01-26 13:43     ` Ferruh Yigit
2024-01-29 18:20       ` [EXT] " Harman Kalra
2024-01-30 23:09         ` Ferruh Yigit [this message]
2024-02-01 10:10           ` Harman Kalra
2024-02-01 10:02 ` [PATCH v5 0/2] multiple representors in one device Harman Kalra
2024-02-01 10:02   ` [PATCH v5 1/2] ethdev: parsing multiple representor devargs string Harman Kalra
2024-02-01 10:02   ` [PATCH v5 2/2] test/devargs: add eth devargs parse cases Harman Kalra
2024-02-01 18:35   ` [PATCH v5 0/2] multiple representors in one device Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9eddad36-4692-4439-aecf-731994bc2c7d@amd.com \
    --to=ferruh.yigit@amd.com \
    --cc=Yuying.Zhang@intel.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=beilei.xing@intel.com \
    --cc=chaoyong.he@corigine.com \
    --cc=dev@dpdk.org \
    --cc=dsosnowski@nvidia.com \
    --cc=hkalra@marvell.com \
    --cc=hyonkim@cisco.com \
    --cc=johndale@cisco.com \
    --cc=matan@nvidia.com \
    --cc=orika@nvidia.com \
    --cc=qi.z.zhang@intel.com \
    --cc=qiming.yang@intel.com \
    --cc=somnath.kotur@broadcom.com \
    --cc=suanmingm@nvidia.com \
    --cc=thomas@monjalon.net \
    --cc=viacheslavo@nvidia.com \
    --cc=wenjun1.wu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).