Hi Ferruh
Please find response inline
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: Wednesday, January 31, 2024 4:40 AM
> To: Harman Kalra <hka...@marvell.com>; dev@dpdk.org; Thomas Monjalon
> <tho...@monjalon.net>; Andrew Rybchenko
> <andrew.rybche...@oktetlabs.ru>; Ajit Khaparde
> <ajit.khapa...@broadcom.com>; Somnath Kotur
> <somnath.ko...@broadcom.com>; John Daley <johnd...@cisco.com>; Hyong
> Youb Kim <hyon...@cisco.com>; Yuying Zhang <yuying.zh...@intel.com>;
> Beilei Xing <beilei.x...@intel.com>; Qiming Yang <qiming.y...@intel.com>; Qi
> Zhang <qi.z.zh...@intel.com>; Wenjun Wu <wenjun1...@intel.com>;
> Dariusz Sosnowski <dsosnow...@nvidia.com>; Viacheslav Ovsiienko
> <viachesl...@nvidia.com>; Ori Kam <or...@nvidia.com>; Suanming Mou
> <suanmi...@nvidia.com>; Matan Azrad <ma...@nvidia.com>; Chaoyong
> He <chaoyong...@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.yi...@amd.com>
> >> Sent: Friday, January 26, 2024 7:13 PM
> >> To: Harman Kalra <hka...@marvell.com>; dev@dpdk.org; Thomas
> Monjalon
> >> <tho...@monjalon.net>; Andrew Rybchenko
> >> <andrew.rybche...@oktetlabs.ru>; Ajit Khaparde
> >> <ajit.khapa...@broadcom.com>; Somnath Kotur
> >> <somnath.ko...@broadcom.com>; John Daley <johnd...@cisco.com>;
> Hyong
> >> Youb Kim <hyon...@cisco.com>; Yuying Zhang
> <yuying.zh...@intel.com>;
> >> Beilei Xing <beilei.x...@intel.com>; Qiming Yang
> >> <qiming.y...@intel.com>; Qi Zhang <qi.z.zh...@intel.com>; Wenjun Wu
> >> <wenjun1...@intel.com>; Dariusz Sosnowski <dsosnow...@nvidia.com>;
> >> Viacheslav Ovsiienko <viachesl...@nvidia.com>; Ori Kam
> >> <or...@nvidia.com>; Suanming Mou <suanmi...@nvidia.com>; Matan
> Azrad
> >> <ma...@nvidia.com>; Chaoyong He <chaoyong...@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
