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 D15C9A0C4B; Tue, 23 Nov 2021 21:02:32 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 562DE40040; Tue, 23 Nov 2021 21:02:32 +0100 (CET) Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) by mails.dpdk.org (Postfix) with ESMTP id 37C134003C for ; Tue, 23 Nov 2021 21:02:31 +0100 (CET) Received: by mail-wm1-f52.google.com with SMTP id r9-20020a7bc089000000b00332f4abf43fso2661189wmh.0 for ; Tue, 23 Nov 2021 12:02:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=j4Tyy74oFSHZSagUFhg0sRmD5x7doil9fjig0XIuv5k=; b=MSdkvUGbNY1UOuNSpaaFBdza6HKusrAH+W0Q3ZDEjlV66XJDuKhq001Dlgk0WxA7OF SFA5RZPQ4CDAa1hY79IBJ4mXhyASd3hmtHNa1B5mTptuDbvhh3D2sK7ToBkyIrv/t5oz csidn8SExQttG7Clj79IuIPGqOUhkZ1iOopQPXEE1QTQJURS3lXItdqaKmJNAQjeeYr0 PewuTvmgYnTTBoaU/nbSgD2Mh30hTjmOi/S3GrpIkqYQcMPOl2W7yZMTDrPQKIM6vdgQ 3bPrhbGB1tsDEKeFXFBZ3z1N5cyY+Adif7YYhVnn/HEeOOlZ3JkekoevlWPkCXBjULjK qOUg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=j4Tyy74oFSHZSagUFhg0sRmD5x7doil9fjig0XIuv5k=; b=Yd4E087sZEIXvYW0IGq0Fi8OQU/o86N5DVuoLE1eU1S9N+E2Vr0XJtQrXpw3X1MNnV j5qvkQsJ0nQKDh2uRD01aG8D9bdxuy82RkIEgleN4fKPa9U1sGKDbqSPfCuk6kLm9CdY pselb9NLdWiKP9AkjR11Tthu8HLzJHruCN0oUSvvI7rzofzfrBTNjaNaAGMC+AcFtBDY 2RBUfbnpPQnTtVLkULY2UK/IVsiK+lhl/5sXL0O0MhYUolYCLjjKdso8valBq0D7Rqcb lypkidF8OcyP2TLytxTeoaY6wpObZTtFzrZcuF5MiMuuhgiVZU+fFll8NSPLYN+ilJN3 zKNA== X-Gm-Message-State: AOAM5307axvRgD8pE30qdueXluaQ8g0xpqPAw34lkytlX7aMWbqrbHjR wTbQsgA1cuwbPBlsUT60CrOJ5g== X-Google-Smtp-Source: ABdhPJxn+IpCnITjL9K1UHsISyyl5ocw1nJDZMQrizy+TfqipQdaDc9wyQuTCBPxb9RBZTKP2XVpXg== X-Received: by 2002:a1c:f018:: with SMTP id a24mr6977106wmb.51.1637697750842; Tue, 23 Nov 2021 12:02:30 -0800 (PST) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id 9sm16598034wry.0.2021.11.23.12.02.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Nov 2021 12:02:29 -0800 (PST) Date: Tue, 23 Nov 2021 21:02:27 +0100 From: Olivier Matz To: "Xueming(Steven) Li" Cc: Lior Margalit , Parav Pandit , "dev@dpdk.org" , "mdr@ashroe.eu" , "david.marchand@redhat.com" Subject: Re: [PATCH] kvargs: fix device iterator match from arguments Message-ID: References: <20211122061250.3220823-1-xuemingl@nvidia.com> <7e1225023de996e63ed041ee312e861982913caf.camel@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 Hello Xueming, On Tue, Nov 23, 2021 at 12:49:32PM +0000, Xueming(Steven) Li wrote: > On Tue, 2021-11-23 at 13:31 +0100, Olivier Matz wrote: > > On Tue, Nov 23, 2021 at 11:25:22AM +0000, Xueming(Steven) Li wrote: > > > On Tue, 2021-11-23 at 11:25 +0100, Olivier Matz wrote: > > > > Hi Xueming, > > > > > > > > On Mon, Nov 22, 2021 at 02:12:50PM +0800, Xueming Li wrote: > > > > > Device iterator RTE_DEV_FOREACH() failed to return devices from > > > > > classifier like "class=vdpa", because matching name from empty kvargs > > > > > returns no result. If device name not specified in kvargs, the function > > > > > should iterate all devices. > > > > > > > > > > This patch allows empty devargs or devargs without name specified. > > > > > > > > > > Fixes: 6aebb942907d ("kvargs: add function to get from key and value") > > > > > Cc: olivier.matz@6wind.com > > > > > > > > > > Signed-off-by: Xueming Li > > > > > --- > > > > > 21.11 specific bug, no copy to stable.org > > > > > --- > > > > > drivers/bus/auxiliary/auxiliary_params.c | 3 +++ > > > > > drivers/bus/vdev/vdev_params.c | 3 +++ > > > > > 2 files changed, 6 insertions(+) > > > > > > > > > > diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c > > > > > index a9c7853ed1d..6a6382961ea 100644 > > > > > --- a/drivers/bus/auxiliary/auxiliary_params.c > > > > > +++ b/drivers/bus/auxiliary/auxiliary_params.c > > > > > @@ -27,6 +27,9 @@ auxiliary_dev_match(const struct rte_device *dev, > > > > > const struct rte_kvargs *kvlist = _kvlist; > > > > > const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME]; > > > > > > > > > > + /* Iterate all devices if name not specified. */ > > > > > + if (kvlist == NULL || rte_kvargs_get(kvlist, key) == NULL) > > > > > + return 0; > > > > > if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) > > > > > return -1; > > > > > > > > > > diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c > > > > > index 37d95395e7a..bab4c0d1d08 100644 > > > > > --- a/drivers/bus/vdev/vdev_params.c > > > > > +++ b/drivers/bus/vdev/vdev_params.c > > > > > @@ -29,6 +29,9 @@ vdev_dev_match(const struct rte_device *dev, > > > > > const struct rte_kvargs *kvlist = _kvlist; > > > > > const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME]; > > > > > > > > > > + /* Iterate all devices if name not specified. */ > > > > > + if (kvlist == NULL || rte_kvargs_get(kvlist, key) == NULL) > > > > > + return 0; > > > > > if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) > > > > > return -1; > > > > > > > > > > > > > > > > > Thank you for spotting and fixing this issue. The patch looks good to > > > > me, but may I suggest an alternative that would avoid to browse the > > > > kvlist twice? It is not yes tested, just for discussion. The idea > > > > is to add an errno for error cases of rte_kvargs_get_with_value() > > > > to identify the different cases. > > > > > > Yes, the code walk the kvlist twice. An alternative complex code could > > > be this: > > > > > > if (kvlist == NULL) > > > return 0; > > > name = rte_kvargs_get(kvlist, key); > > > if (name == NULL) > > > /* Iterate all devices if name not specified. */ > > > return 0; > > > if (strcmp(name, dev->name) != 0) > > > return -1; > > > > Maybe it is equivalent for common usages, but a difference is that > > rte_kvargs_get_with_value(kvlist, "key", "value2") also matches > > a kvlist="key=value1,key=value2" (i.e. the case where the key appears > > several times with different values). > > > > > > > > > > > > --- a/drivers/bus/auxiliary/auxiliary_params.c > > > > +++ b/drivers/bus/auxiliary/auxiliary_params.c > > > > @@ -27,7 +27,9 @@ auxiliary_dev_match(const struct rte_device *dev, > > > > const struct rte_kvargs *kvlist = _kvlist; > > > > const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME]; > > > > > > > > - if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) > > > > + /* if kvlist is valid and contains the key, filter matching devices */ > > > > + if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL && > > > > + rte_errno == ENOENT) > > > > > > rte_errno == ENODEV? we should allow ENOENT - name not specified. > > > > > > > return -1; > > > > I think the patch is correct: the item is filtered only if there is no > > matching key/value tuple, but the key is found. > > For "class=vdpa", name key not specified, all devices should be walked. > the kvargs is "" in such case. > > > > > If kvlist is NULL (rte_errno=EINVAL), or if the key does not exist > > (rte_errno=ENODEV), we return 0. > > > > As a side note, there is an errno ENOKEY, which would be much better > > than ENODEV... but it seems it is not POSIX, so it may not exist on > > FreeBSD or Windows. > > > > > > > > > > return 0; > > > > diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c > > > > index 37d95395e7..0a5a8a9f58 100644 > > > > --- a/drivers/bus/vdev/vdev_params.c > > > > +++ b/drivers/bus/vdev/vdev_params.c > > > > @@ -29,7 +29,9 @@ vdev_dev_match(const struct rte_device *dev, > > > > const struct rte_kvargs *kvlist = _kvlist; > > > > const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME]; > > > > > > > > - if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) > > > > + /* if kvlist is valid and contains the key, filter matching devices */ > > > > + if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL && > > > > + rte_errno == ENOENT) > > > > > > Same, ENODEV? which means name specified but not match. > > > > > > > return -1; > > > > > > > > return 0; > > > > diff --git a/lib/kvargs/rte_kvargs.c b/lib/kvargs/rte_kvargs.c > > > > index 11f624ef14..f1491715bf 100644 > > > > --- a/lib/kvargs/rte_kvargs.c > > > > +++ b/lib/kvargs/rte_kvargs.c > > > > @@ -209,17 +209,28 @@ const char * > > > > rte_kvargs_get_with_value(const struct rte_kvargs *kvlist, const char *key, > > > > const char *value) > > > > { > > > > + int key_found = 0; > > > > unsigned int i; > > > > > > > > - if (kvlist == NULL) > > > > + if (kvlist == NULL) { > > > > + rte_errno = EINVAL; > > > > return NULL; > > > > + } > > > > + > > > > for (i = 0; i < kvlist->count; ++i) { > > > > if (key != NULL && strcmp(kvlist->pairs[i].key, key) != 0) > > > > continue; > > > > + key_found = 1; > > > > if (value != NULL && strcmp(kvlist->pairs[i].value, value) != 0) > > > > continue; > > > > return kvlist->pairs[i].value; > > > > } > > > > + > > > > + if (key_found) > > > > + rte_errno = ENODEV; > > > > + else > > > > + rte_errno = ENOENT; > > > > + > > > > return NULL; > > > > } > > > > > > > > diff --git a/lib/kvargs/rte_kvargs.h b/lib/kvargs/rte_kvargs.h > > > > index 359a9f5b09..3cb22ece48 100644 > > > > --- a/lib/kvargs/rte_kvargs.h > > > > +++ b/lib/kvargs/rte_kvargs.h > > > > @@ -152,8 +152,12 @@ const char *rte_kvargs_get(const struct rte_kvargs *kvlist, const char *key); > > > > * The matching value. If NULL, any value will match. > > > > * > > > > * @return > > > > - * NULL if no key matches the input, > > > > - * a value associated with a matching key otherwise. > > > > + * The value associated with a matching key/value on success. > > > > + * On error, return NULL and rte_errno is set: > > > > + * - EINVAL - kvlist is NULL > > > > + * - ENOENT - no matching key/value tuple, but the key matches with > > > > + * a different value > > > > + * - ENODEV - key is not found in the kvlist > > > > */ > > > > __rte_experimental > > > > const char *rte_kvargs_get_with_value(const struct rte_kvargs *kvlist, > > > > > > > > > > > > Let me know if it would work for you. I can submit a patch if you want. > > > > > > > > > LGTM, let's use your patch. > > > > OK, I'll submit this patch then. Thanks for the feedback. I'm trying my patch, but I run into an issue. Currently, EAL already depends on kvargs. If I add a reference to rte_errno from kvargs, it will require a new dependency to EAL... which is impossible. Using errno instead of rte_errno is not usual in DPDK. Having a typedef for this is not very convenient either. So thinking more about it, your solution looks better. I have a draft of unit test, I'll send an update of your patch including it ASAP. > > > > > > > > > > I can add a unit test for kvargs, but do you know where we could add a > > > > unit test for the dev iterate? > > > > > > Good idea, how about test_devargs.c? > > > > Thanks, I'll have a look. > > > > > > > > > > Thanks, > > > > Olivier > > > >