From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM02-CY1-obe.outbound.protection.outlook.com (mail-cys01nam02on0062.outbound.protection.outlook.com [104.47.37.62]) by dpdk.org (Postfix) with ESMTP id 2ED472BAD for ; Mon, 4 Jul 2016 14:51:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=CAVIUMNETWORKS.onmicrosoft.com; s=selector1-cavium-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=RqbZVxOkTaMBHFlA6i9FQ8cR3AFpvn1jirQKxcKxS+4=; b=BOV0ZbU1u6tt1nw8udVjQqe8M0qrqsctBT0c5xoVvO3oVq2lEanbJZ94NhRHfgO4SW4aM2yOlXwEB2UTZKlBFWjZeCty56RB1mZV0x4TG9fO8RUr+oAWC9RfDtRKIqxrOVX+IECzLKKO10pi8IkQ+Wby+H3fD5xm3zQTRxkgtWs= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Jerin.Jacob@cavium.com; Received: from localhost.localdomain (122.167.47.22) by CY1PR0701MB1726.namprd07.prod.outlook.com (10.163.21.140) with Microsoft SMTP Server (TLS) id 15.1.528.16; Mon, 4 Jul 2016 12:51:02 +0000 Date: Mon, 4 Jul 2016 18:20:42 +0530 From: Jerin Jacob To: Yuanhan Liu CC: , , , , Message-ID: <20160704125041.GA6532@localhost.localdomain> References: <1467028448-8914-1-git-send-email-jerin.jacob@caviumnetworks.com> <1467371814-26754-1-git-send-email-jerin.jacob@caviumnetworks.com> <1467371814-26754-2-git-send-email-jerin.jacob@caviumnetworks.com> <20160704073648.GV2831@yliu-dev.sh.intel.com> <20160704083626.GA12596@localhost.localdomain> <20160704084232.GY2831@yliu-dev.sh.intel.com> <20160704090754.GD12596@localhost.localdomain> <20160704110225.GA2831@yliu-dev.sh.intel.com> <20160704121556.GA5050@localhost.localdomain> <20160704122630.GB26521@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20160704122630.GB26521@yliu-dev.sh.intel.com> User-Agent: Mutt/1.6.1 (2016-04-27) X-Originating-IP: [122.167.47.22] X-ClientProxiedBy: BM1PR01CA0021.INDPRD01.PROD.OUTLOOK.COM (10.163.198.156) To CY1PR0701MB1726.namprd07.prod.outlook.com (10.163.21.140) X-MS-Office365-Filtering-Correlation-Id: 3f8be6ff-97e9-4e1d-09f8-08d3a409dcd2 X-Microsoft-Exchange-Diagnostics: 1; CY1PR0701MB1726; 2:YZKIkdgWyghu3EpejT/RWoqUqZoIwgxGDKdTAGBQ94ck1y5l+vbAuO85YyeuRS09biBukBehwEYigrKDFnPwAsQDZ2NiY4cnBBD9wii8ZMZttAErVsvGayWGTpAIM1y4TVxztqSCA0GJ/Bz9kYC48+NqUKeDxjFWA8tK/UcKIetJ6VnDKytWUkU3Wf9tG3sG; 3:wOi2rvsIrPL1p+yik1LDT5sfWSZ11e5tkGXA3Dkj0Q13dUamTDpQvoaC+fTmBugx/zxp6qFIKIaznPpsd3tUjeXA3/1gLZGwZsI+jMxFFDMXWu8HkMWGjZLox7cMIRDN; 25:IiAfTXbNR0QYBe29+yWNy1MAONQ5ZP7tCB+3vqNUnad7CwjSvIg7PkF2oJBtlZWfem0nC69EWJRNxYU13DrjmOWxB0z7EchAzZNLUSN6ttUfS0YFdwIgCJi+KqxuN/4O4/f0wCagYzMceOvJnc7adDqzMlkMDolx65SlraO90zV/a2HyvlMIOaf33HUWeNVdmuQhOqi7qbAT4rZ2FFM2lTCNiwTe5Ozk7gHGNs/wUU+n6dZ70TcwayeY1KEP0wv8q+Bk9dFOaW+18Lq7zUKH0PrIkYXJIn7a094+/hi7vOb04h/nG+xfm9NSVpEr/CKcTdWcw7fbAAkje4EuFcUP9oZhiTjcFb5RX/BSZwOAsGC7OS0dwHVTj8F3aNeiGh3ekgh8MF/vGZ4Dke/AE26faBCeeKTfqbP+5FVHpqaB2Eo= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:CY1PR0701MB1726; X-Microsoft-Exchange-Diagnostics: 1; CY1PR0701MB1726; 31:4X2rGVYbH7NyqLqYZT4mJG2jq7FZCgb9hP2cZfb1HSSXkyAUV+OqGQjjVPz7quOeEZF+SuUnvIyC45UUgPD1elZwcXruky2e1f6s9QekEwoYmPmEUdkeAN7VIH7RU58bjFB2qfeANwbaoFOEdLD4/iEE3rpj+vWRFGLJKfPQGdXDfZruI6ThMaM2fxJsGApxXx72JrTPnAcq0jpeSPFVJQ==; 20:hPB2ABCuly/e8ycJVAO3ff/hufkN9S4OiMRoAM0g1NtvyividhDwlN4VcBrY6JWzKd+cFSFTsj9VYOHaPh19MIfeE2sVH8nAUVPeYFN7h1xW7XbR3QON7rV6i6+pMZdQ0wnfkN3qO9G3ml1bw2C3YBQzZedelP8d20WgiPLsOPDUSzCxD8jOrculrzbSyMVcizcKg+ucbYy89MM30OD/lCLc04GtfKHJa0iSCAYBv+mOxcMv1j83xYzocIzjMILfJgP6avBTf01JK2KxgSbCrlkBLPVVLjb2zHOtSZO39X6WmsN1c6fMHLCZswdwFuyC/NTFxcLbZm+7VU8PkX6mQiJsB1h9vVfT16xq0+UAY33uqxT5zf1FLPCNb7IcIaR8ECX4+w+npZttiMNcQofj+qCp0uDoXeeCAuVM7Lb8ULbyQ4DFZ/DTlFGs9Ksv8m6Jhv7cZjQHTYT3yZc+xW8kcO7796HBFEka1UfhzKz0QKdOlJX0WBHv/oUmSWwEKn5c63jqtv/Cai7b+6BaapxzT/JJlfcmWL7C/0XK7noFPmAQ1MGmEWafy0wHlqU+zL84f8NxIAT+0M57ryi1UEX5hSbR+mof3zet0i6ZjlM6ki8= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(17755550239193); X-Exchange-Antispam-Report-CFA-Test: BCL:0; PCL:0; RULEID:(601004)(2401047)(5005006)(8121501046)(10201501046)(3002001); SRVR:CY1PR0701MB1726; BCL:0; PCL:0; RULEID:; SRVR:CY1PR0701MB1726; X-Microsoft-Exchange-Diagnostics: 1; CY1PR0701MB1726; 4:wm5j8F1Hjqolv4IcyKJU+GqzRv+xtLNmAmMJbf13Xxb7aLvmaIeObA+3osX9ZYY+li8C5NlXMMi6G8nDqf68Mp4bPCLW9O9L1dp/9ogNfuxswkQhQOaVpXq+J0YKyocg3homgFeGKp4Jlw6L2Wjkgv9PeXPk2cX70l0vVH88fOClKqWUvoSxhkhhDjc8IktVmpJY5EP70qW1gialhd41UHC/AIz2f47Fs0qn4z3J6YN0xfxtV4RauekejL1AakWpwLDhYj5vhPM+ctCg0iFPHKNhS0nIXGg6TiYaOsAFfpKgmRV9CFYV0EAi6Bfm65AXS/DMhRnNc/N9n+by+01R303GACjOcD7lkIgzMjWoTA1ldHhFTrUQyqfBcCdsW7iKVhL143nSVUrYTlRE7GAl3HX/eOYCWsWVXSrsjyek6zU= X-Forefront-PRVS: 0993689CD1 X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(4630300001)(6069001)(6009001)(7916002)(24454002)(189002)(199003)(105586002)(83506001)(586003)(76176999)(61506002)(101416001)(50986999)(54356999)(106356001)(46406003)(23726003)(1076002)(3846002)(6116002)(2906002)(42186005)(68736007)(66066001)(189998001)(33656002)(110136002)(47776003)(92566002)(93886004)(50466002)(4326007)(2950100001)(8676002)(81156014)(81166006)(7846002)(305945005)(7736002)(97756001)(9686002)(77096005)(4001350100001)(97736004)(7099028)(18370500001); DIR:OUT; SFP:1101; SCL:1; SRVR:CY1PR0701MB1726; H:localhost.localdomain; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; Received-SPF: None (protection.outlook.com: cavium.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1; CY1PR0701MB1726; 23:BSvu0Fn7iHgFd2b8e1pCbXeftt8JuOHUStZnce4?= =?us-ascii?Q?hzsbyMcepkYrNXbjnHoupphHAtS3jO+HEKurU3L4LNn+dvbdL/uW7eVrA/3V?= =?us-ascii?Q?6Aj1lXTHwoqTRDBk2KtCLKemwICW703QflKRDg91G6BQ/fz81cz5jagxaebX?= =?us-ascii?Q?QE0if5i0zgDoxzXTi3uNZsG5yFfxzGuAPG9ptimvmUwAwKJSsqxULDl2oOsI?= =?us-ascii?Q?yX6gjuxK1duNzqN4vT6cRBCNYmvZLdkDGvjJUCTGRuv+9W+DvOUq5gfGf6E3?= =?us-ascii?Q?GzEvIq0XSccrAakmJuc2ZcTUbt18wboOPr5Bza6H0Av9yoj5mzTJgs32Zi/T?= =?us-ascii?Q?DuMyFbd6MopgErV+aSEvs1zkIJnJAGme7tbC0i3mb6gcsNaC3YUnYaZRxl1p?= =?us-ascii?Q?VQdrc2m0BQj/RcTRovvGq0OCTfgxSeUhjPXDnHAs/ttTLcMlH9EaV3kyX+ZP?= =?us-ascii?Q?e6i5Kt9C0yz2JBO59JgaRFtIenQkjDdCy/JAZh94ec9qtPwsilQzV7Tqjip3?= =?us-ascii?Q?2PCvlSgTIRlmw4NHzgAVm1+MmRpscXI7m+8Qqh79EzeO4cSdLadoOLHlvQl9?= =?us-ascii?Q?iYuT34njfzGt3cIZUoeH682TCAFJdrERzH1B+Pc7Vt/jMX2OL0OaCCJufcGK?= =?us-ascii?Q?bQEzlO+H5SUZuI8o5X0wjPCn4ZgE5/VKFjSdbecHJRnpapPVV4zjGkinWqpt?= =?us-ascii?Q?fzJ+8rOIz7pXA52gpPOVlQllLPv6kSEfMr/jfnFliIxIsUeCkPloMJC5nFEr?= =?us-ascii?Q?2TyrOhEwK1hk/lie23G9fJbiqd3RkZ6rJmSyWhXr9q5q/d8EIgJdMCSAPwq1?= =?us-ascii?Q?JZR+uTmRFqBuUGcMdYi97a01Heff7SeCYFA8Twsxh4vsOBwL0eUjRR9zNQkT?= =?us-ascii?Q?WcjZdeEh57N0tWBljVPmfdKZr6yBcQLtrASkx2cR0MMTVmypnmyGlsFVNQoW?= =?us-ascii?Q?Mxe60TsgTmkBcSYwojwMP8BaOxwIBiRREtFMdiRvrvAXqB7G2qC86I9lIpoA?= =?us-ascii?Q?w1P8Hb1u3DGBExt+gvorWEl9BQ13oKKm32tyKWApLE6N19B04RM6FA72xOPJ?= =?us-ascii?Q?Rhe7gksRaA0t/sqlWORpvRru93vBO0hOopBx3SUC7rzECi9CxSN5YHjIRzZY?= =?us-ascii?Q?qPdSYHjcYeZMPbGln9ZJ3bmxf8Bazmxb13qS0qYrrfYNbHAhcy/T2zA=3D?= =?us-ascii?Q?=3D?= X-Microsoft-Exchange-Diagnostics: 1; CY1PR0701MB1726; 6:qEVzBEAlPxJ30VMmhPk7Lt3BTId9BjUDt2jW4eq9eNFDQWEnfg1bn4nujOUc/fC9/6dH4xZhnGrS2ZKR3gbjAfZq6YKvaV4nUHj9hLSIlERkuI9rMEPzLWmtzraANXEsDh9IKaxC0D0izsLNktZMx4PKiCGlqGN+gxoFa66BKFck9+gdUSGfDss9tqeFbmIkyzHAmw5VpSqUZJOJj+bpAmYDjT7fTjCJp7npGAX9akLr7wcTxFvIvimWqMLwWZV9X1kHlMKKYD3IteUZ8V5wqyWvpd5uH6SC1wL6shKlj/k=; 5:gRAUheas+Wi4AGMvleAORNC5Zsnl2yGmVF6XuwbqacN6yugYQoBML0RWCGlR8vW+DT3AdjSKQNitQz7ifuZR6YbJ7eCdzZBXu+ESC7CZzuvsZUfZEiExW1cAaZnidUCL5WBTcJx5BmQ4XNFCfEF5SA==; 24:oEvAxx6AznyMSJdlo5SGKBnOG78vQtxUQmQl80PF/MgN+qei8Ewc8KH8202FIZvv2dBETjXDzAwvPTORnrw6p218wVqljdvY+DPt8Bgd5dg=; 7:+zM1V8YgaQIzMZs2SdQf4JbYX/+piCFK2bBDen6t6zaEIR9143LU7W7L49/aKHFLG1/zVnfMzjzJU/RPVtRqtaloJMLwqOsM1HOpS1GmjQX3GhTG5H6c3HExHc9FO9d1cQpnXacFLFb16iuLoK2JgTkvxRl9/HIGsU9kBTHP6Nd0m9MINpDxPYcbxz3Uq5NxQt9KNs4ua1/6jNh2iek3zLVhqkP2HP3F4KG7nSKqxEJ1rEylIROud39ztJ0vZVVl SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jul 2016 12:51:02.9451 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0701MB1726 Subject: Re: [dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Jul 2016 12:51:07 -0000 On Mon, Jul 04, 2016 at 08:26:30PM +0800, Yuanhan Liu wrote: > On Mon, Jul 04, 2016 at 05:45:57PM +0530, Jerin Jacob wrote: > > On Mon, Jul 04, 2016 at 07:02:25PM +0800, Yuanhan Liu wrote: > > > On Mon, Jul 04, 2016 at 02:37:55PM +0530, Jerin Jacob wrote: > > > > On Mon, Jul 04, 2016 at 04:42:32PM +0800, Yuanhan Liu wrote: > > > > > On Mon, Jul 04, 2016 at 02:06:27PM +0530, Jerin Jacob wrote: > > > > > > On Mon, Jul 04, 2016 at 03:36:48PM +0800, Yuanhan Liu wrote: > > > > > > > On Fri, Jul 01, 2016 at 04:46:36PM +0530, Jerin Jacob wrote: > > > > > > > > @@ -494,9 +486,6 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, > > > > > > > > { > > > > > > > > uint8_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_TQ_QUEUE_IDX; > > > > > > > > > > > > > > > > -#ifdef RTE_MACHINE_CPUFLAG_SSSE3 > > > > > > > > - struct virtio_hw *hw = dev->data->dev_private; > > > > > > > > -#endif > > > > > > > > struct virtnet_tx *txvq; > > > > > > > > struct virtqueue *vq; > > > > > > > > uint16_t tx_free_thresh; > > > > > > > > @@ -511,13 +500,14 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, > > > > > > > > } > > > > > > > > > > > > > > > > #ifdef RTE_MACHINE_CPUFLAG_SSSE3 > > > > > > > > + struct virtio_hw *hw = dev->data->dev_private; > > > > > > > > > > > > > > I'd suggest to move above declaration to ... > > > > > > > > > > > > > > > /* Use simple rx/tx func if single segment and no offloads */ > > > > > > > > if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS && > > > > > > > > !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) { > > > > > > > > > > > > > > here: we should try to avoid declaring vars in the middle of a code block. > > > > > > > > > > > > Next patch in this series, moving all rxtx handler selection code to > > > > > > separate function(virtio_update_rxtx_handler()) where declaration comes > > > > > > as first line in the function.i.e the comment is taken care of in the > > > > > > series. > > > > > > > > > > Yes, I saw that. But in principle, each patch is atomic: it's not a > > > > > good idea/practice to introduce issues in path A and then fix it in > > > > > path B. > > > > > > > > In my view it was not an issue as I was removing all possible > > > > conditional compilation flag. If I were to move the declaration to top > > > > then another conditional compilation RTE_MACHINE_CPUFLAG_SSSE3 > > > > flag I need to add around declaring the variable. > > > > > > Nope, I was suggesting to move it inside the "if" block. So, this > > > is actually consistent with what you are trying to do. Besides, it > > > removes an declaration in the middle. > > > > Just to get the clarity on "moving inside the 'if' block" > > > > Are you suggesting to have like below? > > > > #ifdef RTE_MACHINE_CPUFLAG_SSSE3 > > + struct virtio_hw *hw; > > /* Use simple rx/tx func if single segment and no offloads */ > > if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == > > VIRTIO_SIMPLE_FLAGS && > > !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) { > > PMD_INIT_LOG(INFO, "Using simple rx/tx path"); > > dev->tx_pkt_burst = virtio_xmit_pkts_simple; > > dev->rx_pkt_burst = virtio_recv_pkts_vec; > > - use_simple_rxtx = 1; > > + hw = dev->data->dev_private; > > + hw->use_simple_rxtx = 1; > > } > > #endif > > > > > > Instead of following scheme in existing patch, > > > > #ifdef RTE_MACHINE_CPUFLAG_SSSE3 > > + struct virtio_hw *hw = dev->data->dev_private; > > /* Use simple rx/tx func if single segment and no offloads */ > > if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == > > VIRTIO_SIMPLE_FLAGS && > > !vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) { > > PMD_INIT_LOG(INFO, "Using simple rx/tx path"); > > dev->tx_pkt_burst = virtio_xmit_pkts_simple; > > dev->rx_pkt_burst = virtio_recv_pkts_vec; > > - use_simple_rxtx = 1; > > + hw->use_simple_rxtx = 1; > > } > > #endif > > > > > > The former case will have issue as "hw" been used in "if" with vtpci_with_feature. > > Oh, my bad. I overlooked it. Sorry for that! > > > OR > > > > if you meant just floating "struct virtio_hw *hw" without RTE_MACHINE_CPUFLAG_SSSE3 > > then it comes error on non x86 as unused "hw" variable. > > > > If you meant something else then let me know? > > I then prefer to keep the "#ifdef .. #endif" on top then. It will stop > us from offending a minor rule, while you can remove the ugly "#ifdef" > block in the next patch. > > Works to you? OK. As you wish :-) > > --yliu