Re: [PATCH] net/tap: Bug fix to populate fds in secondary process

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/18/2022 10:52 AM, kumaraparameshwaran rathinavel wrote:

Comment moved down, please avoid top posting.

> 
> On Tue, Jan 18, 2022 at 2:40 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 1/18/2022 4:39 AM, Kumara Parameshwaran wrote:
>      >>   static int
>      >>   tap_dev_start(struct rte_eth_dev *dev)
>      >>   {
>      >>        int err, i;
>      >>
>      >> +     tap_mp_req_on_rxtx(dev);
>      >> +
>      >
>      > As for as I understand your logic is primary sends the message to the secondar(y|ies),
>      > so what happens first secondary is started?
>      > ​In case of TAP PMD looks like there is an assumption where primary should be started first. There is an existing check below during the probe function call.
>      > if (!rte_eal_primary_proc_alive(NULL)) {
>      >       TAP_LOG(ERR, "Primary process is missing");
>      >        return -1;
>      > }
>      >
>      > What about secondary sends the message when they are started?
>      > ​IMHO, since primary process setups the queue it should be sufficient for the primary processes to the send the message and secondary need not send anything.
>      >
>      > Also above functions is called by both primary and secondary, what happens when it is
>      > called by secondary? And the logic is not clear, it can be good to add a process type
>      > check to clarify.
>      > ​Sure, these are for tap_intr_handle_set and tap_dev_start functions?
> 
>     I was thinking within the 'tap_dev_start()' function, for 'tap_mp_req_on_rxtx()' call.
> 
>     Not sure how 'tap_intr_handle_set()' is involved, am I missing something.
>> Yes, even I was confused if it  had been the tap_intr_handle_set function.
> 
> In general the tap_dev_start should not be invoked by the secondary and only primary should do it. I referred it to a couple of PMDs and that was the case. Please let me know if I am missing something in my understanding.
> 


Yes, that is the intended usecase, that primary does the start.
But having the check can help documenting the intention, that it is only for primary.