From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) by dpdk.org (Postfix) with ESMTP id 274774CBD for ; Mon, 5 Nov 2018 17:25:15 +0100 (CET) Received: by mail-wm1-f44.google.com with SMTP id v24-v6so9023568wmh.3 for ; Mon, 05 Nov 2018 08:25:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=JNAfs5v5aEHyBDaNELctUwkVwgeROQ3BtHov3yv2EEU=; b=anOkWcGDYbfcf9GLCPSovXVUj1XxIzI5p7NvraEIKbEcc9uoK1G2VaIrKtNLL9w/54 qn+YpaJfN8vmxPSpooBRscZuUX7uf30ttcNPrAEBnKTfHkcEtmcHRmZlycj25In/TRUe Xo229EeaTMtH9HolQMweiGMWv43i8hlimunnLOYTpfg+oEln0AF9RL1lwLxslEY7afX0 441XAViRvnpkBEiMMR5TBY9cV6eNtTiL4hhOJUZeulYYTzLc8eoKDyaHSaXsrgbFE+Ln Tn0WzcpAUvA6xeOaPOW71l0jLmYNYil0D6Z/07knkcHKQ7XLAWWoMmmL3xaZcK15qPbD 7EVA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=JNAfs5v5aEHyBDaNELctUwkVwgeROQ3BtHov3yv2EEU=; b=SUgm4j05JSnbE/+Jbbb0Y1cr2S5uZCQLOx733TI5uMeG0Rsdt65jBBFU9PCXiDLF2z FtqjGvHnnAWyMEH1swlh44yEcs/M2CYL3hNLll/T207zHXJoLWK7BB7Ae9Y2S/aWCn2z TCO0D2svpDZ7fJ96QLOtJk2P6aiRxcnHkFag4S3m/UqsrN2i4o+5KfcP5BgAIn1R3GDF J3ZlQwNRlJONeaZf3bY7VsLlXzbdxxRe2xawF+jAEk523gzLUYTq44iCpSu0L5Wr0ThA G3VlhdNzsATjd1JGLli3PufR8e4vDf1jnbRX5OtueoG0Uxad1pYcrcSDYGwk2zb+x8ig 5Vgg== X-Gm-Message-State: AGRZ1gJOKJC+4rJamcNznTL/yJeQrQDDUftQ2fAlsf3E2MvTLnLt5Mh8 4BoZXClgqvvFlXmOi8auVqypGQ== X-Google-Smtp-Source: AJdET5cZfG2OO82rs2EBd7B3L1AkZZljLMxOqzhFc1PBGE2fBZuSGsL/Jw+MXwwGWr4aj+7MXMT9jA== X-Received: by 2002:a1c:410a:: with SMTP id o10-v6mr6844995wma.19.1541435114190; Mon, 05 Nov 2018 08:25:14 -0800 (PST) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id r1-v6sm9482918wrt.59.2018.11.05.08.25.13 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 05 Nov 2018 08:25:13 -0800 (PST) Date: Mon, 5 Nov 2018 17:24:54 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Thomas Monjalon Cc: "Stojaczyk, Dariusz" , "dev@dpdk.org" Message-ID: <20181105162454.2gluhg5j6evgcihl@bidouze.vm.6wind.com> References: <20181105070447.67700-1-dariusz.stojaczyk@intel.com> <4545148.O9CyC7tLm6@xps> <4918273.sFLXtBqTXr@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4918273.sFLXtBqTXr@xps> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH 2/3] devargs: delay freeing previous devargs when overriding them X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Nov 2018 16:25:15 -0000 On Mon, Nov 05, 2018 at 10:46:39AM +0100, Thomas Monjalon wrote: > 05/11/2018 09:25, Stojaczyk, Dariusz: > > Hi Thomas, > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > 05/11/2018 08:04, Darek Stojaczyk: > > > > -int __rte_experimental > > > > -rte_devargs_insert(struct rte_devargs *da) > > > > +void __rte_experimental > > > > +rte_devargs_insert(struct rte_devargs *da, struct rte_devargs > > > **prev_da) > > > > > > You should update the API section of the release notes. > > > > Even for experimental API? OK, I didn't know it's needed. > > Yes, even for experimental API, the API changes must documented. > > > > > { > > > > - int ret; > > > > + struct rte_devargs *d; > > > > + void *tmp; > > > > + > > > > + *prev_da = NULL; > > > > + TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) { > > > > + if (strcmp(d->bus->name, da->bus->name) == 0 && > > > > + strcmp(d->name, da->name) == 0) { > > > > + TAILQ_REMOVE(&devargs_list, d, next); > > > > + *prev_da = d; > > > > + break; > > > > + } > > > > + } > > > > > > > > - ret = rte_devargs_remove(da); > > > > - if (ret < 0) > > > > - return ret; > > > > > > Why not updating rte_devargs_remove instead of duplicating its code? > > > > We still want to preserve the functionality of rte_devargs_remove. > > rte_devargs_remove does TAILQ_REMOVE + free; rte_devargs_insert does just TAILQ_REMOVE. (I think I also forgot to update rte_devargs_insert documentation, I'll do that in V2) > > Yes, because of the rollback, OK. > Please mention in devargs_insert doc that the old devargs > can be used for rollback. > This is not ok. You are leaking those devargs that are not freed. Once a devargs is inserted, the current API considers them to be managed by EAL - i.e. no one else should manipulate them (get them out of the list or freeing them). They are meant afterward to be linked by rte_device, so the only moment they can be removed is right before a bus scan (which will rebuild the devargs -> device mapping). Returning the prev_devargs from devargs_insert() is a bad construct. Insert only does insertion, not search+insert. Either, devargs_insert() finds a previous occurence, and returns an error EEXIST before aborting, or it forces the insertion. But it should not do something else to avoid a function call. > > Since you've mentioned it: > > Eventually I'd see rte_devargs_remove to accept the exact same devargs parameter that was passed to rte_devargs_insert. Then rte_devargs_remove wouldn't do any sort of lookup. Maybe additional rte_devargs_find(const char *name) could be added for existing cases where the original devargs struct is not available. However, I'm not familiar enough with this code to perform the refactor and am just trying to fix the stuff. Still, how does it sound? > > I think we can keep it as is. > > We can re-think the whole thing in the next release. > I think we should not play with devargs list as we do. > There should be only lists for scanned devices of each bus and that's all. > > > devargs_{insert,remove} currently identifies a devargs from its (bus, name) pair. This pair is unique in the list. Accepting the exact same devargs parameter is not possible, because sometimes you won't have this devargs handle, you can only identify it by the pair above. I think devargs_find() would give the wrong idea (returning a devargs handle that is still in the devargs list: a user should not think it can be manipulated this way). Instead I'd propose: devargs_insert() returns -EEXIST if the devargs identifying pair is found in the list. devargs_extract() will remove this devargs from the list and return it if found. devargs_insert() afterward would work. Once extracted, the devargs is the responsibility of the caller (so should be freed if not needed anymore). devargs_remove() stays the same, finding the devargs and freeing it, or returning >0 if the devargs was not within the list. Remaining issue is that extract() would invalidate a bunch of rte_device pointers, which is not acceptable. Either a back-reference is made from devargs to device, so that rte_devices are updated accordingly, or we must ensure each device bus scan is called right after to make sure the mapping is fixed. -- Gaëtan Rivet 6WIND