Re: [PATCH 2/8] lib/librte_ether: defind RX/TX lock mode

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

On 06/08/2016 09:34 AM, Lu, Wenzhuo wrote:
> Hi Stephen,
> 
> 
>> -----Original Message-----
>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>> Sent: Wednesday, June 8, 2016 10:16 AM
>> To: Lu, Wenzhuo
>> Cc: dev@dpdk.org; Tao, Zhe
>> Subject: Re: [dpdk-dev] [PATCH 2/8] lib/librte_ether: defind RX/TX lock mode
>>
>> On Mon,  6 Jun 2016 13:40:47 +0800
>> Wenzhuo Lu <wenzhuo.lu@intel.com> wrote:
>>
>>> Define lock mode for RX/TX queue. Because when resetting the device we
>>> want the resetting thread to get the lock of the RX/TX queue to make
>>> sure the RX/TX is stopped.
>>>
>>> Using next ABI macro for this ABI change as it has too much impact. 7
>>> APIs and 1 global variable are impacted.
>>>
>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>> Signed-off-by: Zhe Tao <zhe.tao@intel.com>
>>
>> Why does this patch set make a different assumption the rest of the DPDK?
>>
>> The rest of the DPDK operates on the principle that the application is smart
>> enough to stop the device before making changes. There is no equivalent to the
>> Linux kernel RTNL mutex. The API assumes application threads are well behaved
>> and will not try and sabotage each other.
>>
>> If you restrict the reset operation to only being available when RX/TX is stopped,
>> then no lock is needed.
>>
>> The fact that it requires lots more locking inside each device driver implies to me
>> this is not correct way to architect this.

+1

I'm not sure adding locks is the proper way to do.
This is the application responsibility to ensure that:
- control functions are not called concurrently on the same port
- rx/tx functions are not called when the device is stopped/reset/...

However, I do think the usage paradigms of the ethdev api should be
better documented in rte_ethdev.h (ex: which functions can be called
concurrently). This would be a first step.

If we really want a helper API to do that in DPDK, the _next_ step
could be to add them in the ethdev api to achieve this. Maybe
something like (the function names could be better):

- to be called on one control thread:

  rte_eth_stop_rxtx(port)
  rte_eth_start_rxtx(port)

  rte_eth_get_rxtx_state(port)
     -> return "running" if at least one core is inside the rx/tx code
     -> return "stopped" if all cores are outside the rx/tx code

- to be called on dataplane cores:

  /* same than rte_eth_rx_burst(), but checks if rx/tx is allowed
   * first, else do nothing */
  rte_eth_rx_burst_interruptible()
  rte_eth_tx_burst_interruptible()


The code of control thread could be:

  rte_eth_stop_rxtx(port);
  /* wait that all dataplane cores finished their processing */
  while (rte_eth_get_rxtx_state(port) != stopped)
      ;
  rte_eth_some_control_operation(port);
  rte_eth_start_rxtx(port);


I think this could be done without any lock, just with the proper
memory barriers and a per-core status.

But this API may impose a paradigm to the application, and I'm not
sure the DPDK should do that.

Regards,
Olivier