From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by dpdk.space (Postfix) with ESMTP id 30163A0096
for <public@inbox.dpdk.org>; Wed, 5 Jun 2019 13:55:41 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 4A89D1B9DD;
Wed, 5 Jun 2019 13:55:40 +0200 (CEST)
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
by dpdk.org (Postfix) with ESMTP id 494A21B9D7
for <dev@dpdk.org>; Wed, 5 Jun 2019 13:55:38 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from fmsmga006.fm.intel.com ([10.253.24.20])
by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
05 Jun 2019 04:55:37 -0700
X-ExtLoop1: 1
Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.26])
([10.237.221.26])
by fmsmga006.fm.intel.com with ESMTP; 05 Jun 2019 04:55:36 -0700
To: Jakub Grajciar <jgrajcia@cisco.com>, dev@dpdk.org
References: <20190520101841.17708-1-jgrajcia@cisco.com>
<20190531062247.5952-1-jgrajcia@cisco.com>
From: Ferruh Yigit <ferruh.yigit@intel.com>
Openpgp: preference=signencrypt
Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata=
mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy
qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ
+iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9
GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb
+dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF
YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy
ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX
CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1
1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz
cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln
aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQkI71rKFiEE
0jZTh0IuwoTjmYHH+TPrQ98TYR8FAlznMMQFCwkIBwMFFQoJCAsFFgIDAQAACgkQ+TPrQ98T
YR/B9Q//a57esjq996nfZVm7AsUl7zbvhN+Ojity25ib2gcSVVsAN2j6lcQS4hf6/OVvRj3q
CgebJ4o2gXR6X12UzWBJL7NE8Xpc70MvUIe0r11ykurQ9n9jUaWMjxdSqBPF93hU+Z/MZe5M
1rW5O2VJLuTJzkDw3EYUCbHOwPjeaS8Qqj3RI0LYbGthbHBIp9CsjkgsJSjTT5GQ8AQWkE7I
z+hvPx6f1rllfjxFyi4DI3jLhAI+j1Nm+l+ESyoX59HrLTHAvq4RPkLpTnGBj9gOnJ+5sVEr
GE0fcffsNcuMSkpqSEoJCPAHmChoLgezskhhsy0BiU3xlSIj1Dx2XMDerUXFOK3ftlbYNRte
HQy4EKubfZRB8H5Rvcpksom3fRBDcJT8zw+PTH14htRApU9f8I/RamQ7Ujks7KuaB7JX5QaG
gMjfPzHGYX9PfF6KIchaFmAWLytIP1t0ht8LpJkjtvUCSQZ2VxpCXwKyUzPDIF3co3tp90o7
X07uiC5ymX0K0+Owqs6zeslLY6DMxNdt8ye+h1TVkSZ5g4dCs4C/aiEF230+luL1CnejOv/K
/s1iSbXQzJNM7be3FlRUz4FdwsfKiJJF7xYALSBnSvEB04R7I2P2V9Zpudkq6DRT6HZjBeJ1
pBF2J655cdoenPBIeimjnnh4K7YZBzwOLJf2c6u76fe5Ag0EV9ZMvgEQAKc0Db17xNqtSwEv
mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T
qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA
ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/
vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p
iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ
I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH
fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ
qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi
6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl
Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh
HwUCXOcvZgUJBvIWKAAKCRD5M+tD3xNhHxhBD/9toXMIaPIVFd9w1nKsRDM1GE6gZe4jie8q
MJpeHB9O+936fSXA0W2X0het60wJQQ45O8TpTcxpc9nGzcE4MTaLAI3E8TjIXAO0cPqUNLyp
g0DXezmTw5BU+SKZ51+jSKOtFmzJCHOJZQaMeCHD+G3CrdUHQVQBb5AeuH3KFv9ltgDcWsc8
YO70o3+tGHwcEnyXLdrI0q05wV7ncnLdkgVo+VUN4092bNMPwYly1TZWcU3Jw5gczOUEfTY7
sgo6E/sGX3B+FzgIs5t4yi1XOweCAQ/mPnb6uFeNENEFyGKyMG1HtjwBqnftbiFO3qitEIUY
xWGQH23oKscv7i9lT0gg2D+ktzZhVWwHJVY/2vWSB9aCSWChcH2BT+lWrkwSpoPhy+almM84
Qz2wF72/d4ce4L27pSrS+vOXtXHLGOOGcAn8yr9TV0kM4aR+NbGBRXGKhG6w4lY54uNd9IBa
ARIPUhij5JSygxZCBaJKo+X64AHGkk5bXq+f0anwAMNuJXbYC/lz4DEdKmPgQGShOWNs1Y1a
N3cI87Hun/RBVwQ0a3Tr1g6OWJ6xK8cYbMcoR8NZ7L9ALMeJeuUDQR39+fEeHg/6sQN0P0mv
0sL+//BAJphCzDk8ztbrFw+JaPtgzZpRSM6JhxnY+YMAsatJRXA0WSpYP5zzl7yu/GZJIgsv VQ==
Message-ID: <7171944c-edec-e6c9-1795-fdd6e8385377@intel.com>
Date: Wed, 5 Jun 2019 12:55:36 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <20190531062247.5952-1-jgrajcia@cisco.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Subject: Re: [dpdk-dev] [PATCH v10] net/memif: introduce memory interface
(memif) PMD
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On 5/31/2019 7:22 AM, Jakub Grajciar wrote:
> Memory interface (memif), provides high performance
> packet transfer over shared memory.
Almost there, can you please check below comments? I am hoping to merge next
version of the patch.
Thanks,
ferruh
>
> Signed-off-by: Jakub Grajciar <jgrajcia@cisco.com>
<...>
> +static const char *valid_arguments[] = {
> + ETH_MEMIF_ID_ARG,
> + ETH_MEMIF_ROLE_ARG,
> + ETH_MEMIF_PKT_BUFFER_SIZE_ARG,
> + ETH_MEMIF_RING_SIZE_ARG,
> + ETH_MEMIF_SOCKET_ARG,
> + ETH_MEMIF_MAC_ARG,
> + ETH_MEMIF_ZC_ARG,
> + ETH_MEMIF_SECRET_ARG,
> + NULL
> +};
Checkpatch is giving following warning:
WARNING:STATIC_CONST_CHAR_ARRAY: static const char * array should probably be
static const char * const
#1885: FILE: drivers/net/memif/rte_eth_memif.c:39:
+static const char *valid_arguments[] = {
<...>
> +static int
> +rte_pmd_memif_probe(struct rte_vdev_device *vdev)
> +{
> + RTE_BUILD_BUG_ON(sizeof(memif_msg_t) != 128);
> + RTE_BUILD_BUG_ON(sizeof(memif_desc_t) != 16);
> + int ret = 0;
> + struct rte_kvargs *kvlist;
> + const char *name = rte_vdev_device_name(vdev);
> + enum memif_role_t role = MEMIF_ROLE_SLAVE;
> + memif_interface_id_t id = 0;
> + uint16_t pkt_buffer_size = ETH_MEMIF_DEFAULT_PKT_BUFFER_SIZE;
> + memif_log2_ring_size_t log2_ring_size = ETH_MEMIF_DEFAULT_RING_SIZE;
> + const char *socket_filename = ETH_MEMIF_DEFAULT_SOCKET_FILENAME;
> + uint32_t flags = 0;
> + const char *secret = NULL;
> + struct rte_ether_addr *ether_addr = rte_zmalloc("", sizeof(struct rte_ether_addr), 0);
This is a long line, and breaking it won't reduce the readability, can you
please break it. Checkpatch warning:
WARNING:LONG_LINE: line over 80 characters
#2939: FILE: drivers/net/memif/rte_eth_memif.c:1093:
+ struct rte_ether_addr *ether_addr = rte_zmalloc("", sizeof(struct
rte_ether_addr), 0);
<...>
> +static int
> +rte_pmd_memif_remove(struct rte_vdev_device *vdev)
> +{
> + struct rte_eth_dev *eth_dev;
> + int i;
> +
> + eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(vdev));
> + if (eth_dev == NULL)
> + return 0;
> +
> + for (i = 0; i < eth_dev->data->nb_rx_queues; i++)
> + (*eth_dev->dev_ops->rx_queue_release)(eth_dev->data->rx_queues[i]);
> + for (i = 0; i < eth_dev->data->nb_tx_queues; i++)
> + (*eth_dev->dev_ops->rx_queue_release)(eth_dev->data->tx_queues[i]);
Although they point same function, better to use 'dev_ops->tx_queue_release' for
Tx queues.
> +
> + rte_free(eth_dev->process_private);
> + eth_dev->process_private = NULL;
"process_private" is not used in this PMD at all, no need to free it I think.
> +
> + rte_eth_dev_close(eth_dev->data->port_id);
There are two exit path from a PMD:
1) rte_eth_dev_close() API
2) rte_vdev_driver->remove() called by hotplug remove APIs ('rte_dev_remove()'
or 'rte_eal_hotplug_remove()')
Both should clear all PMD allocated resources. Since you are calling
'rte_eth_dev_close() from this .remove() function, it makes sense to move all
resource cleanup to .dev_close (like queue cleanup calls above).