From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 89F02A0C5B; Mon, 9 Aug 2021 18:07:19 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 19D2F4068A; Mon, 9 Aug 2021 18:07:19 +0200 (CEST) Received: from mail-io1-f44.google.com (mail-io1-f44.google.com [209.85.166.44]) by mails.dpdk.org (Postfix) with ESMTP id 87CED4003C for ; Mon, 9 Aug 2021 18:07:17 +0200 (CEST) Received: by mail-io1-f44.google.com with SMTP id e186so26239696iof.12 for ; Mon, 09 Aug 2021 09:07:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=banduracyber-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language; bh=1yYTXAr+o/CstsPDbXisEWK9/FahFm53AHuCieDJpIc=; b=a/p114hjI9Po2pMraySKWkH6FBaHPpCDDnwPhCMlV8rx7ktV9bdh7LHTyr4Ac7vtUc /96vu9bsYqnXyeKWaZKTc65/8h9CJH1DoVH9jJvIm4GrWvArSzcy9JiRki9oYgYWNVIT BPzeCZ08lkaSxcEHjTn9K5qhbeG9BmsHQbvzFoWUoNKM2Uarf+nv4l6cS3TZIWmakvt8 vJagS9RIwqag7WJrDAj71FHYFWxgPc5pTC+Xx+hBYnMOEggG260unxCR8M+Q7m1035WO kNLV0TVXiSNHtjlfoOkAF7F9XZqtJI+6hAecBs1jPYzoc47mWoxyDnPfE3DE2l7wdRVO yeTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=1yYTXAr+o/CstsPDbXisEWK9/FahFm53AHuCieDJpIc=; b=O2OyLhEIXAS/uwuiW2KLU6KVphLm3Mh2WmHs/sHI7lz4XwBEqcoe9Fs4TxlEwRehdG Og5U1dJxS8gQinTNScb0F/YGFffHT3DXuYncth7uttboxizt8S7FWKRjEW+Ie5RfyOSE AdnG4QgYv44kzOohcf2N55FlxLjLGLHf0wVP2YRXwU7oc37zkogR2Dl/Wn5Rln3nEIUg lV9GunkwwtMOg6qJ/fS6VVItrmESZ9Qyf0iEaWO6xtKYXRoZiMWHsEDGBVgL5zIat85x ScQ/6KiU/tmx3od6hrV2+7suGf7YV3lBoGbVqfujrBITfn5pVL65bxoj1dEEWLT9tX7k SmFQ== X-Gm-Message-State: AOAM532CgUv++ho+TzrvyIrlvGVtgDPJdw4im48YZrjElx+5tSKNu5vZ hpDCWh0njJGsEdLzyWrkbkSWRw== X-Google-Smtp-Source: ABdhPJwPdLE1f3u1AbDHmlur0vEFpQzQyPDEgPHlkOafLJ62cfZA2dUTsedNXfgbYeqK6qeuHZaNcQ== X-Received: by 2002:a5e:9743:: with SMTP id h3mr51840ioq.52.1628525236751; Mon, 09 Aug 2021 09:07:16 -0700 (PDT) Received: from [192.168.0.8] (dyn-69-88-36-66.myactv.net. [69.88.36.66]) by smtp.gmail.com with ESMTPSA id 79sm11401182iou.18.2021.08.09.09.07.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 09 Aug 2021 09:07:16 -0700 (PDT) To: Long Li , Stephen Hemminger Cc: "dev@dpdk.org" , "stable@dpdk.org" References: <20210726170040.25155-1-jonathan.erb@banduracyber.com> From: Jonathan Erb Message-ID: Date: Mon, 9 Aug 2021 12:07:15 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [dpdk-dev] [PATCH v2] bus/vmbus: Fix crash when handling packets in secondary process X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Would it be possible to resolve the lack of subchannels in secondary processes as follows: 1. Create the following function in vmbus_uio.c to obtain the count of subchannels created by the primary: int vmbus_uio_get_num_subchan(struct vmbus_channel *primary) {     const struct rte_vmbus_device *dev = primary->device;     char chan_path[PATH_MAX];     struct dirent *ent;     DIR *chan_dir;     int err;     int subchan_cnt = 0;     snprintf(chan_path, sizeof(chan_path),          "%s/%s/channels",          SYSFS_VMBUS_DEVICES, dev->device.name);     chan_dir = opendir(chan_path);     if (!chan_dir) {         VMBUS_LOG(ERR, "cannot open %s: %s",               chan_path, strerror(errno));         return 0;     }     while ((ent = readdir(chan_dir))) {         unsigned long relid, subid, monid;         char *endp;         if (ent->d_name[0] == '.')             continue;         errno = 0;         relid = strtoul(ent->d_name, &endp, 0);         if (*endp || errno != 0 || relid > UINT16_MAX) {             VMBUS_LOG(NOTICE, "not a valid channel relid: %s",                   ent->d_name);             continue;         }         subchan_cnt++;     }     closedir(chan_dir);     //Less one for primary channel     return subchan_cnt - 1; } 2. Then change the bottom of vmbus_uio_map_secondary() to be simply:     /* fd is not needed in secondary process, close it */     close(fd);     if (vmbus_chan_create(dev, dev->relid, 0,                     dev->monitor_id, &dev->primary)) {         VMBUS_LOG(ERR, "cannot create primary channel");         return -1;     }     int subchannels_cnt = vmbus_uio_get_num_subchan(dev->primary);     for (i = 0; i < subchannels_cnt; i++) {         if(vmbus_uio_get_subchan(dev->primary, &chan))             return -1; STAILQ_INSERT_TAIL(&dev->primary->subchannel_list, chan, next);     }     return 0; } In this way the secondary processes detect the number of subchannels created by the primary, then perform their own mappings as needed. All this can occur before rte_eal_init has completed. Jonathan On 7/26/21 6:16 PM, Long Li wrote: >> Subject: [PATCH v2] bus/vmbus: Fix crash when handling packets in >> secondary process >> >> Have secondary processes construct their own copy of primary channel with >> own mappings. >> >> Remove vmbus_channel primary ptr from struct mapped_vmbus_resource >> as its not used. >> >> Populate virtual memory address "addr" in struct rte_mem_resource for >> secondary processes as netvsc will attempt to reference it thus causing a >> crash. It was initialized for primary processes but not for secondary. >> Cc: stable@dpdk.org >> >> Signed-off-by: Jonathan Erb >> --- >> v2: >> * Remove unnecessary check for NULL pointer before call to rte_free() per >> reviwer comment. >> >> drivers/bus/vmbus/private.h | 1 - >> drivers/bus/vmbus/vmbus_channel.c | 4 +--- >> drivers/bus/vmbus/vmbus_common_uio.c | 14 +++++++++----- >> 3 files changed, 10 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h >> index 528d60a42f..746212bd5f 100644 >> --- a/drivers/bus/vmbus/private.h >> +++ b/drivers/bus/vmbus/private.h >> @@ -42,7 +42,6 @@ struct mapped_vmbus_resource { >> >> rte_uuid_t id; >> int nb_maps; >> - struct vmbus_channel *primary; >> struct vmbus_map maps[VMBUS_MAX_RESOURCE]; >> char path[PATH_MAX]; >> }; >> diff --git a/drivers/bus/vmbus/vmbus_channel.c >> b/drivers/bus/vmbus/vmbus_channel.c >> index f67f1c438a..119b9b367e 100644 >> --- a/drivers/bus/vmbus/vmbus_channel.c >> +++ b/drivers/bus/vmbus/vmbus_channel.c >> @@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device >> *device, >> >> err = vmbus_chan_create(device, device->relid, 0, >> device->monitor_id, new_chan); >> - if (!err) { >> + if (!err) >> device->primary = *new_chan; >> - uio_res->primary = *new_chan; >> - } >> >> return err; >> } >> diff --git a/drivers/bus/vmbus/vmbus_common_uio.c >> b/drivers/bus/vmbus/vmbus_common_uio.c >> index 8582e32c1d..83c56b6fa2 100644 >> --- a/drivers/bus/vmbus/vmbus_common_uio.c >> +++ b/drivers/bus/vmbus/vmbus_common_uio.c >> @@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device >> *dev) >> fd, offset, >> uio_res->maps[i].size, 0); >> >> - if (mapaddr == uio_res->maps[i].addr) >> + if (mapaddr == uio_res->maps[i].addr) { >> + dev->resource[i].addr = mapaddr; >> continue; /* successful map */ >> + } >> >> if (mapaddr == MAP_FAILED) >> VMBUS_LOG(ERR, >> @@ -88,9 +90,9 @@ vmbus_uio_map_secondary(struct rte_vmbus_device >> *dev) >> /* fd is not needed in secondary process, close it */ >> close(fd); >> >> - dev->primary = uio_res->primary; >> - if (!dev->primary) { >> - VMBUS_LOG(ERR, "missing primary channel"); >> + if (vmbus_chan_create(dev, dev->relid, 0, >> + dev->monitor_id, &dev->primary)) { >> + VMBUS_LOG(ERR, "cannot create primary channel"); >> return -1; >> } > Looking at this closer, I don't think it will work for subchannels in the secondary process. > > The code after is: > > STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) { > if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) { > VMBUS_LOG(ERR, "cannot map secondary subchan"); > return -1; > } > } > > Because at this time, the primary channel is just created, "subchannel_list" should be NULL. The secondary process ends up running without subchannels. In the primary process, "subchannel_list" are populated when it calls hn_dev_configure(), so it is good. > > Sorry I didn't spot this earlier. The best way to fix this is to use rte memory functions for allocating vmbus device. In this way they can be properly mapped to the secondary process. But rte memory functions are not available at the time vmbus device is probed. I haven't been able to find a good way to fix this. Will keep looking. > > Thanks, > Long > > >> @@ -211,8 +213,10 @@ vmbus_uio_unmap_resource(struct >> rte_vmbus_device *dev) >> return; >> >> /* secondary processes - just free maps */ >> - if (rte_eal_process_type() != RTE_PROC_PRIMARY) >> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) { >> + rte_free(dev->primary); >> return vmbus_uio_unmap(uio_res); >> + } >> >> TAILQ_REMOVE(uio_res_list, uio_res, next); >> >> -- >> 2.17.1