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:

quoted

On Mon, Jul 04, 2016 at 07:02:25PM +0800, Yuanhan Liu wrote:

quoted

On Mon, Jul 04, 2016 at 02:37:55PM +0530, Jerin Jacob wrote:

quoted

On Mon, Jul 04, 2016 at 04:42:32PM +0800, Yuanhan Liu wrote:

quoted

On Mon, Jul 04, 2016 at 02:06:27PM +0530, Jerin Jacob wrote:

quoted

On Mon, Jul 04, 2016 at 03:36:48PM +0800, Yuanhan Liu wrote:

quoted

On Fri, Jul 01, 2016 at 04:46:36PM +0530, Jerin Jacob wrote:

quoted

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

quoted

 	/* 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!

quoted

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