Re: [PATCH v2] net/memif: default to physical socket

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 12/7/2022 3:56 PM, Nathan Skrzypczak wrote:
> Hi Ferruh,
> 

Hi Nathan,

> 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.
> 

Not sure if regular socket is easier to handle, or users prefer regular
sockets, we need more input on these.

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

I don't have a preference about the default config, but I also don't
have enough justification for changing the current behavior.


@Jakub, as driver maintainer, do you have any preference?


> Cheers
> -Nathan
> 
> [0] 2f865ed07b
> 
> Le mer. 7 déc. 2022 à 16:14, Ferruh Yigit <ferruh.yigit@amd.com
> <mailto:ferruh.yigit@amd.com>> a écrit :
> 
>     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=yes 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=no' 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 <nathan.skrzypczak@gmail.com
>     <mailto:nathan.skrzypczak@gmail.com>>
>     > ---
>     >  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=1024", "Size of single packet buffer", "2048", "uint16_t"
>     >     "rsize=11", "Log2 of ring size. If rsize is 10, actual ring
>     size is 1024", "10", "1-14"
>     >     "socket=/tmp/memif.sock", "Socket filename",
>     "/tmp/memif.sock", "string len 108"
>     > -   "socket-abstract=no", "Set usage of abstract socket address",
>     "yes", "yes|no"
>     > +   "socket-abstract=no", "Is the socket an abstract socket",
>     "no", "yes|no"
>     >     "mac=01:23:45:ab:cd:ef", "Mac address", "01:ab:23:cd:45:ef", ""
>     >     "secret=abc123", "Secret is an optional security option, which
>     if specified, must be matched by peer", "", "string len 24"
>     >     "zero-copy=yes", "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 =
>     > @@ -1139,7 +1140,9 @@ memif_connect_client(struct rte_eth_dev *dev)
>     > 
>     >       ret = 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 |= ETH_MEMIF_FLAG_SOCKET_ABSTRACT;
>     > -
>     >       kvlist = rte_kvargs_parse(rte_vdev_device_args(vdev),
>     valid_arguments);
>     > 
>     >       /* parse parameters */
>