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 C4C28A034C; Mon, 12 Dec 2022 12:16:52 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 66E684021D; Mon, 12 Dec 2022 12:16:52 +0100 (CET) Received: from mail-ej1-f43.google.com (mail-ej1-f43.google.com [209.85.218.43]) by mails.dpdk.org (Postfix) with ESMTP id 3CAA740156 for ; Wed, 7 Dec 2022 16:56:15 +0100 (CET) Received: by mail-ej1-f43.google.com with SMTP id kw15so11601478ejc.10 for ; Wed, 07 Dec 2022 07:56:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=eDM4fBbVJW2TRHVTMPbMjC9cIftOZO8OCFM+5T7EF8U=; b=ACzzou2uxXLNlREubBZLH465IiS4yuQJiu4e9sJEClkkk8qSBWm172YTjSZuryNzH8 hAC7XfcG5Rj0QPwm/RH/yfN7QAkEYMfBTZWr1wsVkf0/hypCsN6K1L1Ww4xRy1YSXIys NIOOQ+F+de95c3cTaopdlj3mJjoZpGghDq2ergmJJ3NV0K6yCuqcQtLcCzYJCpnxETD8 l21p99FJeO11nHLu72OeyHzcyoqvAHNjnd45KPIuabO9T5QwifBT1aePCPXbtkL/EgNv cjFj95FUNH8sLK4VcByVK+AYr2OGE8urzk/kAftat5IkJUp9BXLKzDeiE+aFlCGq3Bjf +hvA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=eDM4fBbVJW2TRHVTMPbMjC9cIftOZO8OCFM+5T7EF8U=; b=xB//G6uhMFmLxI1H+Qyg3rvJxnIuvl2KyFHYrSzQfASbebXsiblnshU736w18y+RJr CKiRtFTMN1VQJXoaxTIyrclt41+AetYriQ97ANTvQEGa6ZfemYrcG+V0zEilB98lKXvA Q0WPsq5jZR4amlttDwNAMMdm8FpH/yhK7Cl9g3BLozYr0nMlT7BvyfVs4L6zz7aerCsZ XzT87t0WFlDOXE20CFx85f4r+BuEF9ebx0cUtmKxQCev2Y1mRLy16WOqIsz1+HxFc71n j19TSbXV2nKLhiNG17uIWs5kO+zSLnJ3D0/4Bnr5xcVmOdvbndAXUvM/JNSAIqmmGnIn ixng== X-Gm-Message-State: ANoB5plpQOPB41rZIFfvR+IkylKFnD4Ghm1IGYbLKIGUvx78uG9L3QSU JiGsIT6RjumXVhw4dmDsgD+um7TgP34CFUTd3fM= X-Google-Smtp-Source: AA0mqf4XsXK80Z3Tka2Iuoj0GM5X/E2PREvdEySPwT3Mm9/q57X+LnfUmlTvZ8/7wPnBRco3RFiBgLf7Fi74QA9qzEQ= X-Received: by 2002:a17:906:c40e:b0:7c0:f4ee:8f10 with SMTP id u14-20020a170906c40e00b007c0f4ee8f10mr11600066ejz.449.1670428574675; Wed, 07 Dec 2022 07:56:14 -0800 (PST) MIME-Version: 1.0 References: <20221010104038.15867-1-nathan.skrzypczak@gmail.com> <20221017121246.9721-1-nathan.skrzypczak@gmail.com> <1520db83-9e0f-9973-f2e1-d2a91a3b3104@amd.com> In-Reply-To: <1520db83-9e0f-9973-f2e1-d2a91a3b3104@amd.com> From: Nathan Skrzypczak Date: Wed, 7 Dec 2022 16:56:03 +0100 Message-ID: Subject: Re: [PATCH v2] net/memif: default to physical socket To: Ferruh Yigit Cc: andrew.rybchenko@oktetlabs.ru, Jakub Grajciar , dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000d0fa1205ef3ef555" X-Mailman-Approved-At: Mon, 12 Dec 2022 12:16:51 +0100 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 --000000000000d0fa1205ef3ef555 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Ferruh, Thank you for your reply, On the potential confusion for users of the DPDK memif PMD : when defaulting to abstract sockets was added in [0] (v20.10 release) it did change the existing behavior, so reverting it would restore the old behavior. Also abstract sockets are quite a unusual feature in linux (a 0byte prefixed string...), so I'm expecting most users of memif to just use regular sockets because they're way easier to handle. Also my guess is it will probably bug people if the way to use regular sockets is to pass `abstract-socket=3Dfalse` to the PMD config. What do you think ? Cheers -Nathan [0] 2f865ed07b Le mer. 7 d=C3=A9c. 2022 =C3=A0 16:14, Ferruh Yigit = a =C3=A9crit : > On 10/17/2022 1:12 PM, Nathan Skrzypczak wrote: > > This patch changes the default value of the memif abstract > > socket flag to false. The implementation of memif with > > abstract sockets made abstract-socket=3Dyes the > > default in [0] which might confuse users. > > > > Hi Nathan, > > OK to update logs to clarify nature of the socket, > > but why do you think making abstract socket default socket type confuses > the users? > > > This patches makes 'socket-abstract=3Dno' the new default, > > and adds warnings mentioning the nature of the socket > > concerned in an attempt to avoid future foot-gunning. > > > > [0] commit 2f865ed07bb6 ("net/memif: use abstract socket address") > > > > Signed-off-by: Nathan Skrzypczak > > --- > > doc/guides/nics/memif.rst | 2 +- > > drivers/net/memif/memif_socket.c | 7 +++++-- > > drivers/net/memif/rte_eth_memif.c | 3 --- > > 3 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/doc/guides/nics/memif.rst b/doc/guides/nics/memif.rst > > index aca843640b..43d1cd1b38 100644 > > --- a/doc/guides/nics/memif.rst > > +++ b/doc/guides/nics/memif.rst > > @@ -43,7 +43,7 @@ client. > > "bsize=3D1024", "Size of single packet buffer", "2048", "uint16_t" > > "rsize=3D11", "Log2 of ring size. If rsize is 10, actual ring size = is > 1024", "10", "1-14" > > "socket=3D/tmp/memif.sock", "Socket filename", "/tmp/memif.sock", > "string len 108" > > - "socket-abstract=3Dno", "Set usage of abstract socket address", "ye= s", > "yes|no" > > + "socket-abstract=3Dno", "Is the socket an abstract socket", "no", > "yes|no" > > "mac=3D01:23:45:ab:cd:ef", "Mac address", "01:ab:23:cd:45:ef", "" > > "secret=3Dabc123", "Secret is an optional security option, which if > specified, must be matched by peer", "", "string len 24" > > "zero-copy=3Dyes", "Enable/disable zero-copy client mode. Only > relevant to client, requires '--single-file-segments' eal argument", "no"= , > "yes|no" > > diff --git a/drivers/net/memif/memif_socket.c > b/drivers/net/memif/memif_socket.c > > index 4700ce2e77..5344e60147 100644 > > --- a/drivers/net/memif/memif_socket.c > > +++ b/drivers/net/memif/memif_socket.c > > @@ -939,7 +939,8 @@ memif_socket_create(char *key, uint8_t listener, > bool is_abstract) > > if (ret < 0) > > goto error; > > > > - MIF_LOG(DEBUG, "Memif listener socket %s created.", > sock->filename); > > + MIF_LOG(DEBUG, "Memif listener %s socket %s created.", > > + is_abstract ? "abstract" : "", sock->filename); > > > > /* Allocate interrupt instance */ > > sock->intr_handle =3D > > @@ -1139,7 +1140,9 @@ memif_connect_client(struct rte_eth_dev *dev) > > > > ret =3D connect(sockfd, (struct sockaddr *)&sun, sunlen); > > if (ret < 0) { > > - MIF_LOG(ERR, "Failed to connect socket: %s.", > pmd->socket_filename); > > + MIF_LOG(ERR, "Failed to connect %s socket: %s.", > > + pmd->flags & ETH_MEMIF_FLAG_SOCKET_ABSTRACT ? > "abstract" : "", > > + pmd->socket_filename); > > goto error; > > } > > > > diff --git a/drivers/net/memif/rte_eth_memif.c > b/drivers/net/memif/rte_eth_memif.c > > index 5b5cae34ea..81e502ccaf 100644 > > --- a/drivers/net/memif/rte_eth_memif.c > > +++ b/drivers/net/memif/rte_eth_memif.c > > @@ -1823,9 +1823,6 @@ rte_pmd_memif_probe(struct rte_vdev_device *vdev) > > MIF_LOG(WARNING, "Failed to register mp action callback: > %s", > > strerror(rte_errno)); > > > > - /* use abstract address by default */ > > - flags |=3D ETH_MEMIF_FLAG_SOCKET_ABSTRACT; > > - > > kvlist =3D rte_kvargs_parse(rte_vdev_device_args(vdev), > valid_arguments); > > > > /* parse parameters */ > > --000000000000d0fa1205ef3ef555 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Ferruh,

Thank you for you= r reply,=C2=A0

On the potential confusion for user= s of the DPDK memif PMD : when defaulting to abstract sockets was added in = [0] (v20.10 release)
it did change the existing behavior, so= reverting it would restore the old behavior.
Also abstract = sockets are quite a unusual feature in linux (a 0byte prefixed string...), = so I'm expecting most users of memif to just use
regular sock= ets because they're way easier to handle.

Also= my guess is it will probably bug people if the way to use regular sockets = is to pass `abstract-socket=3Dfalse` to the PMD config.
What = do you think ?

Cheers
-Nathan
<= div>
[0] 2f865ed07b

Le=C2=A0mer. 7 d=C3=A9c. 2022 =C3=A0= =C2=A016:14, Ferruh Yigit <ferru= h.yigit@amd.com> a =C3=A9crit=C2=A0:
On 10/17/2022 1:12 PM, Nathan Skrzypczak wrote:=
> This patch changes the default value of the memif abstract
> socket flag to false. The implementation of memif with
> abstract sockets made abstract-socket=3Dyes the
> default in [0] which might confuse users.
>

Hi Nathan,

OK to update logs to clarify nature of the socket,

but why do you think making abstract socket default socket type confuses the users?

> This patches makes 'socket-abstract=3Dno' the new default,
> and adds warnings mentioning the nature of the socket
> concerned in an attempt to avoid future foot-gunning.
>
> [0] commit 2f865ed07bb6 ("net/memif: use abstract socket address&= quot;)
>
> Signed-off-by: Nathan Skrzypczak <nathan.skrzypczak@gmail.com>
> ---
>=C2=A0 doc/guides/nics/memif.rst=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 2 += -
>=C2=A0 drivers/net/memif/memif_socket.c=C2=A0 | 7 +++++--
>=C2=A0 drivers/net/memif/rte_eth_memif.c | 3 ---
>=C2=A0 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/doc/guides/nics/memif.rst b/doc/guides/nics/memif.rst
> index aca843640b..43d1cd1b38 100644
> --- a/doc/guides/nics/memif.rst
> +++ b/doc/guides/nics/memif.rst
> @@ -43,7 +43,7 @@ client.
>=C2=A0 =C2=A0 =C2=A0"bsize=3D1024", "Size of single pack= et buffer", "2048", "uint16_t"
>=C2=A0 =C2=A0 =C2=A0"rsize=3D11", "Log2 of ring size. If= rsize is 10, actual ring size is 1024", "10", "1-14&qu= ot;
>=C2=A0 =C2=A0 =C2=A0"socket=3D/tmp/memif.sock", "Socket = filename", "/tmp/memif.sock", "string len 108"
> -=C2=A0 =C2=A0"socket-abstract=3Dno", "Set usage of abs= tract socket address", "yes", "yes|no"
> +=C2=A0 =C2=A0"socket-abstract=3Dno", "Is the socket an= abstract socket", "no", "yes|no"
>=C2=A0 =C2=A0 =C2=A0"mac=3D01:23:45:ab:cd:ef", "Mac addr= ess", "01:ab:23:cd:45:ef", ""
>=C2=A0 =C2=A0 =C2=A0"secret=3Dabc123", "Secret is an opt= ional security option, which if specified, must be matched by peer", &= quot;", "string len 24"
>=C2=A0 =C2=A0 =C2=A0"zero-copy=3Dyes", "Enable/disable z= ero-copy client mode. Only relevant to client, requires '--single-file-= segments' eal argument", "no", "yes|no"
> diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memi= f_socket.c
> index 4700ce2e77..5344e60147 100644
> --- a/drivers/net/memif/memif_socket.c
> +++ b/drivers/net/memif/memif_socket.c
> @@ -939,7 +939,8 @@ memif_socket_create(char *key, uint8_t listener, b= ool is_abstract)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0goto error;
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0MIF_LOG(DEBUG, "= Memif listener socket %s created.", sock->filename);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0MIF_LOG(DEBUG, "= Memif listener %s socket %s created.",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0is_abstract ? "abstract" : "", sock->filename= );
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Allocate inte= rrupt instance */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sock->intr_ha= ndle =3D
> @@ -1139,7 +1140,9 @@ memif_connect_client(struct rte_eth_dev *dev) >=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D connect(sockfd, (struct sockaddr *)&= amp;sun, sunlen);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0MIF_LOG(ERR, "Fa= iled to connect socket: %s.", pmd->socket_filename);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0MIF_LOG(ERR, "Fa= iled to connect %s socket: %s.",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pmd->= ;flags & ETH_MEMIF_FLAG_SOCKET_ABSTRACT ? "abstract" : "= ",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pmd->= ;socket_filename);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto error;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0
> diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte= _eth_memif.c
> index 5b5cae34ea..81e502ccaf 100644
> --- a/drivers/net/memif/rte_eth_memif.c
> +++ b/drivers/net/memif/rte_eth_memif.c
> @@ -1823,9 +1823,6 @@ rte_pmd_memif_probe(struct rte_vdev_device *vdev= )
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0MIF_LOG(WARNING,= "Failed to register mp action callback: %s",
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0strerror(rte_errno));
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0/* use abstract address by default */
> -=C2=A0 =C2=A0 =C2=A0flags |=3D ETH_MEMIF_FLAG_SOCKET_ABSTRACT;
> -
>=C2=A0 =C2=A0 =C2=A0 =C2=A0kvlist =3D rte_kvargs_parse(rte_vdev_device_= args(vdev), valid_arguments);
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/* parse parameters */

--000000000000d0fa1205ef3ef555--