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 224D7A0C4D; Sun, 5 Sep 2021 23:06:06 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A4F2240042; Sun, 5 Sep 2021 23:06:05 +0200 (CEST) Received: from mail-il1-f169.google.com (mail-il1-f169.google.com [209.85.166.169]) by mails.dpdk.org (Postfix) with ESMTP id F37974003D for ; Sun, 5 Sep 2021 23:06:03 +0200 (CEST) Received: by mail-il1-f169.google.com with SMTP id w7so4565626ilj.12 for ; Sun, 05 Sep 2021 14:06:03 -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-transfer-encoding:content-language; bh=Yuxbstznv/clIFCAGzuJWrflckr/EPXCAvHmNN+JVUs=; b=Jn+MRJRJb6TwqJKRkugbSdUF+DTwTtRIgUZsyLYF/aLofMyh9jePP01LL8SrP+dHm1 p+7+CPQ+Y9q5uhl4QA7mDfiuqUaDPD2ia93JoS8wVRpsQXQYZZVnKCN2Fpi4jjY2X2ge YWNFsknTu3pHjpSDeIu9ULDTPt0H8/toDGhem0wghXzwj9gFmCTj1QFJYIT54JIHHqpr zDrwUUSp8/UTXnghGa8SrMz491e2EWXyN+psu5W783/Wiok+gWju32pxGvFid8XohNyn g/0uS3itgHJpk0rTfS8NL9DcRMi8RnnqdHLmlWefG+zn24Vh9QuPjLvrlyaDWG3VtWbJ 5Ysg== 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-transfer-encoding :content-language; bh=Yuxbstznv/clIFCAGzuJWrflckr/EPXCAvHmNN+JVUs=; b=RNvtLzBGBDk7GbylxS6B3UDYv9YpO1jjmd0MajKz628Jqf0SV50rrOPaS8TKEe4hO4 fQy88exk33wKP6BEMoxPLrbazlXL9TcFVK953KAz+mg2uzFyzi5OQ9rNNqtfHhcF3FAB lOK+Pviso4rJeoRyduPw/ptKLXtCC7kMwuK2Up00V67/OhrKQyO9cMV/gb03S4Hs8ykL RFSdvbQbGOa1knSI/zvCJ7lTG+f8hAD+/hhb+MvNgof21/OyGPst3N+dkvLij32AZUJv Z8wtytfw+JRWto1BeC/HohWmPzebm94v3t4DxwPCvOsEfClKEv8u2HNGPgTF/R3Kc9Su pijg== X-Gm-Message-State: AOAM533mYIzRjBmFN667PwA/CQM6UZ5V5AMH+Z0k3ZNsjXsLyissCG0z qzXQ81fBr/7LohbJvw598W+qgA== X-Google-Smtp-Source: ABdhPJx3303jZKOiCsqvdEt2wap0JvyKx6kiRSq/vrQ8c/MZaVSrDCUNvSgpEZ6G1zeH4amqaTuoEA== X-Received: by 2002:a05:6e02:1bab:: with SMTP id n11mr6417106ili.85.1630875963073; Sun, 05 Sep 2021 14:06:03 -0700 (PDT) Received: from [192.168.0.2] (dyn-69-88-36-66.myactv.net. [69.88.36.66]) by smtp.gmail.com with ESMTPSA id g8sm3336475ild.31.2021.09.05.14.06.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 05 Sep 2021 14:06:02 -0700 (PDT) To: Long Li , Stephen Hemminger Cc: "dev@dpdk.org" , "stable@dpdk.org" References: <20210726170040.25155-1-jonathan.erb@banduracyber.com> <4387890b-e4da-9a93-a142-dfd976b291b0@banduracyber.com> From: Jonathan Erb Message-ID: <6775c190-f07a-059c-ec6a-0165643fb94c@banduracyber.com> Date: Sun, 5 Sep 2021 17:05:58 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US 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" Long, I have applied the patch and have verified that I can successfully send packets on queue 1 in a secondary process without issue. I believe this exercises the use of vmbus subchannels in secondary processes. Seems fine to me. Thanks. On 8/31/21 2:01 AM, Long Li wrote: > I posted a patch to the mailing list. > > http://patchwork.dpdk.org/project/dpdk/patch/1630389404-13468-1-git-send-email-longli@linuxonhyperv.com/ > > Jonathan, please let me know if this has fixed the crash you are seeing. > > Long > >> Subject: Re: [dpdk-dev] [PATCH v2] bus/vmbus: Fix crash when handling packets >> in secondary process >> >> In the 2nd part, the mapped addresses for the sub-channels in the second >> process should be the same as those mapped addresses in the primary process. >> >> You are correct on seting br->vbr to mapaddr. (To the same addresses used by >> the primary process) This can be done by recording the mapped addresses for all >> the sub-channels in the primary process to vmbus_tailq. Later when the >> secondary process re-creates those channels, it can call >> vmbus_uio_find_resource() to find out the addresses for the same sub-channels >> mapped by the primary process. >> >> >> From: Jonathan Erb >> Sent: Friday, August 13, 2021 3:10 PM >> To: Long Li ; Stephen Hemminger >> >> Cc: dev@dpdk.org; stable@dpdk.org >> Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in >> secondary process >> >> >> The first part makes sense. There are several ways to iterate over all the >> subchannels. >> >> I'm probably not fully understanding the second part. >> vmbus_uio_map_secondary_subchan() wants to map to br->vbr but that does >> not appear to be initialized anywhere. In fact, it looks like we should set br->vbr >> to mapaddr which is what vmbus_uio_map_subchan() does. >> >> Jonathan >> On 8/11/21 6:06 PM, Long Li wrote: >> I think the code is on the right track. >> >> Instead of using vmbus_uio_get_num_subchan() and calling >> vmbus_uio_get_subchan() on each channel, you can just create a new function >> vmbus_uio_get_secondary_subchan(). This function goes through all >> subchannels and map ring buffers to the same addresses used by the primary >> process. >> >> In the original code, vmbus_uio_map_secondary_subchan() has a check for "if >> (mapaddr == ring_buf)". If the address is mapped to somewhere else, the >> address won't work for the secondary process. So we need to keep this check. >> >> From: Stephen Hemminger >> >> Sent: Wednesday, August 11, 2021 8:26 AM >> To: Jonathan Erb >> ; >> Long Li >> Cc: dev@dpdk.org; >> stable@dpdk.org >> Subject: RE: [PATCH v2] bus/vmbus: Fix crash when handling packets in >> secondary process >> >> Looks fine, only comments would be to keep to original style and add check if >> primary channel not found? >> >> From: Jonathan Erb >> > >> Sent: Monday, August 9, 2021 9:07 AM >> To: Long Li >; Stephen >> Hemminger > >> Cc: dev@dpdk.org; >> stable@dpdk.org >> Subject: Re: [PATCH v2] bus/vmbus: Fix crash when handling packets in >> secondary process >> >> You don't often get email from >> jonathan.erb@banduracyber.com. >> Learn why this is important >> >> 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 >>