[dpdk-dev] Question on examples/multi_process app

[Harish Patil]

>>
>>
>>
>>
>>
>>Hi Harish,
>>
>>> >> >
>>> >> >> -----Original Message-----
>>> >> >> From: Richardson, Bruce
>>> >> >> Sent: Wednesday, March 23, 2016 11:45 AM
>>> >> >> To: Ananyev, Konstantin
>>> >> >> Cc: Harish Patil; dev at dpdk.org
>>> >> >> Subject: Re: [dpdk-dev] Question on examples/multi_process app
>>> >> >>
>>> >> >> On Wed, Mar 23, 2016 at 11:09:17AM +0000, Ananyev, Konstantin
>>>wrote:
>>> >> >> > Hi everyone,
>>> >> >> >
>>> >> >> > > -----Original Message-----
>>> >> >> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Bruce
>>> >> >>Richardson
>>> >> >> > > Sent: Tuesday, March 22, 2016 9:38 PM
>>> >> >> > > To: Harish Patil
>>> >> >> > > Cc: dev at dpdk.org
>>> >> >> > > Subject: Re: [dpdk-dev] Question on examples/multi_process
>>>app
>>> >> >> > >
>>> >> >> > > On Tue, Mar 22, 2016 at 08:03:42PM +0000, Harish Patil wrote:
>>> >> >> > > > Hi,
>>> >> >> > > > I have a question regarding symmetric_mp and mp_server
>>> >> >>applications under
>>> >> >> > > > examples/multi_process. In those apps,
>>> >> >>rte_eth_promiscuous_enable() is
>>> >> >> > > > called before rte_eth_dev_start(). Is this the correct way
>>>to
>>> >> >>initialize
>>> >> >> > > > the port/device? As per the description in
>>> >> >> > > > http://dpdk.org/doc/api/rte__ethdev_8h.html:
>>> >> >> > > >
>>> >> >> > > > "The functions exported by the application Ethernet API to
>>> >>setup
>>> >> >>a device
>>> >> >> > > > designated by its port identifier must be invoked in the
>>> >> >>following order:
>>> >> >> > > >
>>> >> >> > > > * rte_eth_dev_configure()
>>> >> >> > > > * rte_eth_tx_queue_setup()
>>> >> >> > > > * rte_eth_rx_queue_setup()
>>> >> >> > > > * rte_eth_dev_start()
>>> >> >> > > >
>>> >> >> > > > Then, the network application can invoke, in any order, the
>>> >> >>functions
>>> >> >> > > > exported by the Ethernet API to get the MAC address of a
>>>given
>>> >> >>device, to
>>> >> >> > > > get the speed and the status of a device physical link, to
>>> >> >> > > > receive/transmit [burst of] packets, and so on.”
>>> >> >> > > >
>>> >> >> > > > So should I consider this as an application issue or
>>>whether
>>> >>the
>>> >> >>PMD is
>>> >> >> > > > expected to handle it? If PMD is to handle it, then should
>>>the
>>> >> >>PMD be:
>>> >> >> > > >
>>> >> >> > > > 1) Rejecting the Promisc config? OR
>>> >> >> > > > 2) Cache the config and apply when dev_start() is called at
>>> >>later
>>> >> >>point?
>>> >> >> >
>>> >> >> > Yes as I remember 2) is done.
>>> >> >> > dev_start() invokes rte_eth_dev_config_restore(), which
>>>restores
>>> >> >> > promisc mode, mac addresses, etc.
>>> >> >> >
>>> >> >> > > >
>>> >> >> > > > Thanks,
>>> >> >> > > > Harish
>>> >> >> > > >
>>> >> >> > > Good question. I think most/all of the Intel adapters, - for
>>> >>which
>>> >> >>the app was
>>> >> >> > > originally written, way back in the day when there were only
>>>2
>>> >>PMDs
>>> >> >>in DPDK :)
>>> >> >> > > - will handle the promiscuous mode call either before or
>>>after
>>> >>the
>>> >> >>dev start.
>>> >> >> > > Assuming that's the case, and if it makes life easier for
>>>other
>>> >> >>driver writers,
>>> >> >> > > we should indeed standardize on one supported way of doing
>>> >>things -
>>> >> >>the way
>>> >> >> > > specified in the documentation being that one way, I would
>>>guess.
>>> >> >> > >
>>> >> >> > > So, e1000, ixgbe maintainers - do you see any issues with
>>>forcing
>>> >> >>the promiscuous
>>> >> >> > > mode set API to be called after the call to dev_start()?
>>> >> >> >
>>> >> >> > Not sure, why do we need to enforce that restriction?
>>> >> >> > Is there any problem with current way?
>>> >>
>>> >> Yes, at least with the our driver/firmware interface. The
>>>port/device
>>> >> bring-up is carried out in a certain order which requires port
>>>config
>>> >>like
>>> >> promisc mode is called after dev_start().
>>> >>
>>> >> >>
>>> >> >> It complicates things for driver writers is all,
>>> >> >
>>> >> >Not sure how?
>>> >> >All this replay is done at rte_ethdev layer.
>>> >> >Honestly, so far I don't remember any complaint about promisc
>>>on/off.
>>> >> >
>>> >> >> and conflicts slightly with
>>> >> >> what is stated in the docs.
>>> >> >
>>> >> >Update the docs? :)
>>> >>
>>> >> Anyway, please let me know what you guys decide? If the app is
>>>changed
>>> >> then nothing needs to done on driver side. Otherwise I have to think
>>>of
>>> >> how to handle this.
>>> >>
>>> >
>>> >So you are saying that for your device if
>>>dev_ops->promiscuous_enable()
>>> >is called before dev_ops->dev_start(), it would cause  a problem
>>>right?
>>> >Konstantin
>>> >
>>> >
>>> 
>>> Yes, with the way it is implemented currently it would pose a problem.
>>> Please note that it can be addressed in the driver, not an issue.
>>>However,
>>> I wanted to be sure if the app behavior is correct. Either ways, please
>>> let me know - I can take care of both.
>>
>>If that's a real HW limitation, then my opinion yes, we probably better
>>address it.
>No its not a HW limitation. It can be handled.
>
>>Though not sure what is the best way:
>>1) just update the docs and rely on users to read them carefully and
>>write the
>>   proper code   
>>2) Inside rte_eth_promiscuous_enable/disable check for dev_started flag,
>>and if it is not set either
>>	a) return an error or
>>	b) update data->promiscuous, but don't invoke
>>dev_ops->promiscuous_enable()
>>	and rely on rte_eth_dev_config_restore() to set it later.
>>Wonder what do people think?
>>
>>BTW, what about other settings?
>>MAC addresses, multicast mode, etc?
>>Are all these things affected too?
>Probably yes. I feel I should fix the driver - that seems more
>appropriate. Please confirm.
>>
>>Konstantin
>>
>
>
Konstantin/Bruce,
On a somewhat related note:

- Should rte_eth_dev_mac_addr_add() be calling
*dev->dev_ops->mac_addr_add() if promiscuous mode is already set?

- Secondly, adding/deleting mac_addr APIs does not have return values and
expect PMD to succeed which may not be the case always.

typedef void (*eth_mac_addr_remove_t)(struct rte_eth_dev *dev, uint32_t
index);
/**< @internal Remove MAC address from receive address register */

typedef void (*eth_mac_addr_add_t)(struct rte_eth_dev *dev,
								struct ether_addr *mac_addr,
								uint32_t index,
								uint32_t vmdq);

Would it be good to modify API to accept return values?


Thanks,
Harish