On 11/15/2018 04:38 PM, Ophir Munk wrote:
> Hi Kevin,
> Thanks for this RFC. Please find comments inline.
>
>> -----Original Message-----
>> From: Kevin Traynor [mailto:ktray...@redhat.com]
>> Sent: Thursday, November 08, 2018 8:37 PM
>> To: d...@openvswitch.org; Ophir Munk <ophi...@mellanox.com>;
>> ian.sto...@intel.com; i.maxim...@samsung.com
>> Cc: Kevin Traynor <ktray...@redhat.com>
>> Subject: [RFC dpdk-latest 2/2] netdev-dpdk: Replace
>> rte_eth_dev_attach/detach.
>>
>> rte_eth_dev_attach/detach have been removed from DPDK 18.11. Replace
>> them with rte_dev_probe/remove.
>>
>
> I have submitted a patch on the same issue, see [1].
> Please suggest how to unify our patches.
>
Hi Ophir,
I looked through your patch and it is trying to do two things:
1. update OVS to use DPDK 18.11
2. enable additional functionality/representors etc in OVS based on
using DPDK 18.11
I don't think that needs to be all in one patch and there isn't really
any throw away work in doing 1. on it's own. My suggestion is that we
proceed with 1. through the patches I sent, and then (or in parallel)
you can send patches to cover 2.
What do people think?
thanks,
Kevin.
>> Signed-off-by: Kevin Traynor <ktray...@redhat.com>
>> ---
>> lib/netdev-dpdk.c | 29 +++++++++++++++++------------
>> 1 file changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>> 10c4879a1..190d50007 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -1351,5 +1351,5 @@ netdev_dpdk_destruct(struct netdev *netdev) {
>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>> - char devname[RTE_ETH_NAME_MAX_LEN];
>> + struct rte_eth_dev_info dev_info;
>>
>> ovs_mutex_lock(&dpdk_mutex);
>> @@ -1360,8 +1360,9 @@ netdev_dpdk_destruct(struct netdev *netdev)
>> if (dev->attached) {
>> rte_eth_dev_close(dev->port_id);
>> - if (rte_eth_dev_detach(dev->port_id, devname) < 0) {
>> + rte_eth_dev_info_get(dev->port_id, &dev_info);
>
> I suggest having a direct access to the device (rather than accessing
> dev_info).
> rte_eth_devices[dev->port_id].device;
>
>> + if (dev_info.device && !rte_dev_remove(dev_info.device)) {
>
> With dpdk 18.11 an rte device can be used by multiple eth devices (one to
> many relationship).
> Remark: in OVS terms an eth device is a dpdk port_id.
> When calling rte_dev_remove() all the eth devices are closed and the PCI
> device is detached.
> We need to count the number of eth devices used by the rte device. Only when
> closing the last eth device
> shoud we call rte_dev_remove (it is like a reference count algorithm).
> For example: if we add to OVS 2 ports belonging the same rte device (e.g. the
> same PCI bus address) then after deleting one of the ports we should not
> implicitly delete the other port.
> This case is handled in [1]
>
>> + VLOG_INFO("Device '%s' has been detached", dev->devargs);
>> + } else {
>> VLOG_ERR("Device '%s' can not be detached", dev->devargs);
>> - } else {
>> - VLOG_INFO("Device '%s' has been detached", devname);
>> }
>> }
>> @@ -1645,5 +1646,5 @@ netdev_dpdk_process_devargs(struct
>> netdev_dpdk *dev,
>> char *name;
>> dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
>> -
>> + struct rte_dev_iterator iterator;
>> if (strncmp(devargs, "class=eth,mac=", 14) == 0) {
>> new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]);
>> @@ -1653,8 +1654,12 @@ netdev_dpdk_process_devargs(struct
>> netdev_dpdk *dev,
>> || !rte_eth_dev_is_valid_port(new_port_id)) {
>> /* Device not found in DPDK, attempt to attach it */
>> - if (!rte_eth_dev_attach(devargs, &new_port_id)) {
>> + if (!rte_dev_probe(devargs)) {
>> /* Attach successful */
>> dev->attached = true;
>> VLOG_INFO("Device '%s' attached to DPDK", devargs);
>> + RTE_ETH_FOREACH_MATCHING_DEV(new_port_id, devargs,
>> &iterator) {
>> + rte_eth_iterator_cleanup(&iterator);
>
> +1
> Good point which I missed!
> Indeed we should free resources by calling the rte_eth_iterator_cleanup().
>
>> + break;
>> + }
>> } else {
>
> It seems with this patch the code still has the call:
> rte_eth_dev_get_port_by_name().
> This call is no more relevant when using representors. Previously a PCI
> address (e.g. 0000:08.00:0) was used in devargs.
> With dpdk 18.11 a device devargs became a regular expression string, (e.g.
> 0000:08.00:0,representor=[1]) which is different than the device name
> assigned by the PMD.
> In other words: previously devargs and device name where identical. Starting
> from dpdk 18.11 they are not.
> This case is handled in [1].
>
They are all valid comments, but as per above, I was only trying to
cover 1. in these patches.
>> /* Attach unsuccessful */ @@ -3206,6 +3211,6 @@
>> netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> char *response;
>> dpdk_port_t port_id;
>> - char devname[RTE_ETH_NAME_MAX_LEN];
>> struct netdev_dpdk *dev;
>> + struct rte_eth_dev_info dev_info;
>>
>> ovs_mutex_lock(&dpdk_mutex);
>> @@ -3226,9 +3231,9 @@ netdev_dpdk_detach(struct unixctl_conn *conn,
>> int argc OVS_UNUSED,
>> rte_eth_dev_close(port_id);
>>
>> - ret = rte_eth_dev_detach(port_id, devname);
>> - if (ret < 0) {
>> - response = xasprintf("Device '%s' can not be detached", argv[1]);
>> - goto error;
>> - }
>> + rte_eth_dev_info_get(port_id, &dev_info);
>> + if (!dev_info.device || rte_dev_remove(dev_info.device)) {
>> + response = xasprintf("Device '%s' can not be detached",
>> argv[1]);
>> + goto error;
>> + }
>>
>
> Before calling ret_dev_remove() we should check if there are no more ports
> which use the same device.
> If there are - we should fail the detach command with an appropriate error
> message to the user.
> Please see [1].
>
>> response = xasprintf("Device '%s' has been detached", argv[1]);
>> --
>> 2.19.0
>
> [1]
> https://patchwork.ozlabs.org/patch/997879/
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
