On 26.05.2017 16:38, O Mahony, Billy wrote:
> Hi Ilya,
>
> This patch does not apply to head of master, currently "c899576
> build-windows: cccl fail compilation on Wimplicit-function-declaration".
Hmm. I'm able to apply it using 'git am'. Have you applied first patch of this
series?
> I'll don't have any comments on the code right now but if you can tell me the
> commit it's based on I'll check it out.
Originally, these patches are made on top of
126fb3e8abc2 ("ovn-ctl: Start ovn-northd even if ovsdb-servers are not running")
but they are still applicable on top of current master.
Bets regards, Ilya Maximets.
>
> Thanks,
> Billy
>
>> -----Original Message-----
>> From: [email protected] [mailto:ovs-dev-
>> [email protected]] On Behalf Of Ilya Maximets
>> Sent: Friday, May 19, 2017 2:38 PM
>> To: [email protected]; Daniele Di Proietto <[email protected]>;
>> Darrell Ball <[email protected]>
>> Cc: Ilya Maximets <[email protected]>; Heetae Ahn
>> <[email protected]>
>> Subject: [ovs-dev] [PATCH v4 2/3] netdev-dpdk: Fix device leak on port
>> deletion.
>>
>> Currently, once created device in dpdk will exist forever even after del-port
>> operation untill we manually call 'ovs-appctl netdev-dpdk/detach <name>',
>> where <name> is not the port's name but the name of dpdk eth device or pci
>> address.
>>
>> Few issues with current implementation:
>>
>> 1. Different API for usual (system) and DPDK devices.
>> (We have to call 'ovs-appctl netdev-dpdk/detach' each
>> time after 'del-port' to actually free the device)
>> This is a big issue mostly for virtual DPDK devices.
>>
>> 2. Follows from 1:
>> For DPDK devices 'del-port' leads just to
>> 'rte_eth_dev_stop' and subsequent 'add-port' will
>> just start the already existing device. Such behaviour
>> will not reset the device to initial state as it could
>> be expected. For example: virtual pcap pmd will continue
>> reading input file instead of reading it from the beginning.
>>
>> 3. Follows from 2:
>> After execution of the following commands 'port1' will be
>> configured with the 'old-options' while 'ovs-vsctl show'
>> will show us 'new-options' in dpdk-devargs field:
>>
>> ovs-vsctl add-port port1 -- set interface port1 type=dpdk \
>> options:dpdk-devargs=<eth_pmd_name1>,<old-options>
>> ovs-vsctl del-port port1
>> ovs-vsctl add-port port1 -- set interface port1 type=dpdk \
>> options:dpdk-devargs=<eth_pmd_name1>,<new-options>
>>
>> 4. Follows from 1:
>> Not detached device consumes 'port_id'. Since we have very
>> limited number of 'port_id's (32 in common case) this may
>> lead to quick exhausting of id pool and inability to add any
>> other port.
>>
>> To avoid above issues we need to detach all the attached devices on port
>> destruction.
>> appctl 'netdev-dpdk/detach' removed because not needed anymore.
>>
>> We need to use internal 'attached' variable to track ports on which
>> rte_eth_dev_attach() was called and returned successfully to avoid closing
>> and detaching devices that do not support hotplug or by any other reason
>> attached using the 'dpdk-extra' cmdline options.
>>
>> CC: Ciara Loftus <[email protected]>
>> Fixes: 55e075e65ef9 ("netdev-dpdk: Arbitrary 'dpdk' port naming")
>> Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs
>> (vdevs)")
>> Signed-off-by: Ilya Maximets <[email protected]>
>> ---
>> Documentation/howto/dpdk.rst | 5 ++-
>> lib/netdev-dpdk.c | 72
>> ++++++++++++--------------------------------
>> 2 files changed, 22 insertions(+), 55 deletions(-)
>>
>> diff --git a/Documentation/howto/dpdk.rst
>> b/Documentation/howto/dpdk.rst index 3bd9e07..7c06239 100644
>> --- a/Documentation/howto/dpdk.rst
>> +++ b/Documentation/howto/dpdk.rst
>> @@ -342,10 +342,9 @@ Then it can be attached to OVS::
>> $ ovs-vsctl add-port br0 dpdkx -- set Interface dpdkx type=dpdk \
>> options:dpdk-devargs=0000:01:00.0
>>
>> -It is also possible to detach a port from ovs, the user has to remove the
>> -port
>> using the del-port command, then it can be detached using::
>> +Detaching will be performed while processing del-port command::
>>
>> - $ ovs-appctl netdev-dpdk/detach 0000:01:00.0
>> + $ ovs-vsctl del-port dpdkx
>>
>> This feature is not supported with VFIO and does not work with some NICs.
>> For more information please refer to the `DPDK Port Hotplug Framework diff
>> --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1586e41..a41679f 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -360,6 +360,9 @@ struct netdev_dpdk {
>> /* Device arguments for dpdk ports */
>> char *devargs;
>>
>> + /* If true, device was attached by rte_eth_dev_attach(). */
>> + bool attached;
>> +
>> /* In dpdk_list. */
>> struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>>
>> @@ -853,6 +856,7 @@ common_construct(struct netdev *netdev, unsigned
>> int port_no,
>> dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
>> ovsrcu_index_init(&dev->vid, -1);
>> dev->vhost_reconfigured = false;
>> + dev->attached = false;
>>
>> ovsrcu_init(&dev->qos_conf, NULL);
>>
>> @@ -998,10 +1002,21 @@ static void
>> netdev_dpdk_destruct(struct netdev *netdev) {
>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>> + char devname[RTE_ETH_NAME_MAX_LEN];
>>
>> ovs_mutex_lock(&dpdk_mutex);
>>
>> rte_eth_dev_stop(dev->port_id);
>> +
>> + if (dev->attached) {
>> + rte_eth_dev_close(dev->port_id);
>> + if (rte_eth_dev_detach(dev->port_id, devname) < 0) {
>> + VLOG_ERR("Device '%s' can not be detached", dev->devargs);
>> + } else {
>> + VLOG_INFO("Device '%s' detached", devname);
>> + }
>> + }
>> +
>> free(dev->devargs);
>> common_destruct(dev);
>>
>> @@ -1113,7 +1128,8 @@ netdev_dpdk_lookup_by_port_id(int port_id) }
>>
>> static int
>> -netdev_dpdk_process_devargs(const char *devargs, char **errp)
>> +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
>> + const char *devargs, char **errp)
>> {
>> /* Get the name up to the first comma. */
>> char *name = xmemdup0(devargs, strcspn(devargs, ",")); @@ -1125,6
>> +1141,7 @@ netdev_dpdk_process_devargs(const char *devargs, char
>> **errp)
>> /* Device not found in DPDK, attempt to attach it */
>> if (!rte_eth_dev_attach(devargs, &new_port_id)) {
>> /* Attach successful */
>> + dev->attached = true;
>> VLOG_INFO("Device '%s' attached to DPDK", devargs);
>> } else {
>> /* Attach unsuccessful */
>> @@ -1211,7 +1228,8 @@ netdev_dpdk_set_config(struct netdev *netdev,
>> const struct smap *args,
>> * is valid */
>> if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
>> && rte_eth_dev_is_valid_port(dev->port_id))) {
>> - int new_port_id = netdev_dpdk_process_devargs(new_devargs,
>> errp);
>> + int new_port_id = netdev_dpdk_process_devargs(dev,
>> new_devargs,
>> + errp);
>> if (!rte_eth_dev_is_valid_port(new_port_id)) {
>> err = EINVAL;
>> } else if (new_port_id == dev->port_id) { @@ -2438,53 +2456,6 @@
>> netdev_dpdk_set_admin_state(struct unixctl_conn *conn, int argc,
>> unixctl_command_reply(conn, "OK");
>> }
>>
>> -static void
>> -netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> - const char *argv[], void *aux OVS_UNUSED)
>> -{
>> - int ret;
>> - char *response;
>> - uint8_t port_id;
>> - char devname[RTE_ETH_NAME_MAX_LEN];
>> - struct netdev_dpdk *dev;
>> -
>> - ovs_mutex_lock(&dpdk_mutex);
>> -
>> - if (!rte_eth_dev_count() || rte_eth_dev_get_port_by_name(argv[1],
>> - &port_id)) {
>> - response = xasprintf("Device '%s' not found in DPDK", argv[1]);
>> - goto error;
>> - }
>> -
>> - dev = netdev_dpdk_lookup_by_port_id(port_id);
>> - if (dev) {
>> - response = xasprintf("Device '%s' is being used by interface '%s'. "
>> - "Remove it before detaching",
>> - argv[1], netdev_get_name(&dev->up));
>> - goto error;
>> - }
>> -
>> - 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;
>> - }
>> -
>> - response = xasprintf("Device '%s' has been detached", argv[1]);
>> -
>> - ovs_mutex_unlock(&dpdk_mutex);
>> - unixctl_command_reply(conn, response);
>> - free(response);
>> - return;
>> -
>> -error:
>> - ovs_mutex_unlock(&dpdk_mutex);
>> - unixctl_command_reply_error(conn, response);
>> - free(response);
>> -}
>> -
>> /*
>> * Set virtqueue flags so that we do not receive interrupts.
>> */
>> @@ -2760,9 +2731,6 @@ netdev_dpdk_class_init(void)
>> unixctl_command_register("netdev-dpdk/set-admin-state",
>> "[netdev] up|down", 1, 2,
>> netdev_dpdk_set_admin_state, NULL);
>> - unixctl_command_register("netdev-dpdk/detach",
>> - "pci address of device", 1, 1,
>> - netdev_dpdk_detach, NULL);
>>
>> ovsthread_once_done(&once);
>> }
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
