RE: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Harman Kalra <hkalra@marvell.com>]

Hi Ferruh

Please find response inline

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Wednesday, January 31, 2024 4:40 AM
> 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: Re: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor
> devargs string
> 
> 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.
 
Yes, it is planned for this release. We have already pushed 2 versions and received
comments. V3 is ready with all V2 comments addressed, I will push by EOD.


> 
> >
> >>
> >> 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?

Thanks for the suggestion, I have updated devargs UT with valid/invalid devarg
patterns. Kindly review V5. I tried to cover most of the use case, please let me
know if any important case I missed.

Thanks
Harman


> 
> >
> >>
> >> 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