Hi Ferruh Please find response inline
> -----Original Message----- > From: Ferruh Yigit <[email protected]> > Sent: Wednesday, January 31, 2024 4:40 AM > To: Harman Kalra <[email protected]>; [email protected]; Thomas Monjalon > <[email protected]>; Andrew Rybchenko > <[email protected]>; Ajit Khaparde > <[email protected]>; Somnath Kotur > <[email protected]>; John Daley <[email protected]>; Hyong > Youb Kim <[email protected]>; Yuying Zhang <[email protected]>; > Beilei Xing <[email protected]>; Qiming Yang <[email protected]>; Qi > Zhang <[email protected]>; Wenjun Wu <[email protected]>; > Dariusz Sosnowski <[email protected]>; Viacheslav Ovsiienko > <[email protected]>; Ori Kam <[email protected]>; Suanming Mou > <[email protected]>; Matan Azrad <[email protected]>; Chaoyong > He <[email protected]> > 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 <[email protected]> > >> Sent: Friday, January 26, 2024 7:13 PM > >> To: Harman Kalra <[email protected]>; [email protected]; Thomas > Monjalon > >> <[email protected]>; Andrew Rybchenko > >> <[email protected]>; Ajit Khaparde > >> <[email protected]>; Somnath Kotur > >> <[email protected]>; John Daley <[email protected]>; > Hyong > >> Youb Kim <[email protected]>; Yuying Zhang > <[email protected]>; > >> Beilei Xing <[email protected]>; Qiming Yang > >> <[email protected]>; Qi Zhang <[email protected]>; Wenjun Wu > >> <[email protected]>; Dariusz Sosnowski <[email protected]>; > >> Viacheslav Ovsiienko <[email protected]>; Ori Kam > >> <[email protected]>; Suanming Mou <[email protected]>; Matan > Azrad > >> <[email protected]>; Chaoyong He <[email protected]> > >> 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
