[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
On Mon, Jul 04, 2016 at 06:20:42PM +0530, Jerin Jacob wrote: > > > 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 :-) Thank you! --yliu
[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
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?
--yliu
[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
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.
--yliu
> Hope this justifies the reason. If you are not convinced then let me know,
> if will add the change in next revision.
>
> Jerin
>
> >
> > --yliu
[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
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
[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
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.
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?
>
> --yliu
>
> > Hope this justifies the reason. If you are not convinced then let me know,
> > if will add the change in next revision.
> >
> > Jerin
> >
> > >
> > > --yliu
[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
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.
--yliu
[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
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.
--yliu
[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
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.
Hope this justifies the reason. If you are not convinced then let me know,
if will add the change in next revision.
Jerin
>
> --yliu
[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
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.
>
> --yliu
[dpdk-dev] [PATCH v2 1/3] virtio: conditional compilation cleanup
Removed unnecessary compile time dependency on "use_simple_rxtx".
Signed-off-by: Jerin Jacob
---
drivers/net/virtio/Makefile | 3 ---
drivers/net/virtio/virtio_pci.h | 1 +
drivers/net/virtio/virtio_rxtx.c| 28 +---
drivers/net/virtio/virtio_rxtx.h| 3 +--
drivers/net/virtio/virtio_rxtx_simple.c | 8 ++--
drivers/net/virtio/virtio_user_ethdev.c | 1 +
6 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
index 3020b68..b9b0d8d 100644
--- a/drivers/net/virtio/Makefile
+++ b/drivers/net/virtio/Makefile
@@ -50,10 +50,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtqueue.c
SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_pci.c
SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx.c
SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
-
-ifeq ($(findstring
RTE_MACHINE_CPUFLAG_SSSE3,$(CFLAGS)),RTE_MACHINE_CPUFLAG_SSSE3)
SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
-endif
ifeq ($(CONFIG_RTE_VIRTIO_USER),y)
SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_user/vhost_user.c
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index dd7693f..b8295a7 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -253,6 +253,7 @@ struct virtio_hw {
uint8_t use_msix;
uint8_t started;
uint8_t modern;
+ uint8_t use_simple_rxtx;
uint8_t mac_addr[ETHER_ADDR_LEN];
uint32_tnotify_off_multiplier;
uint8_t *isr;
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index a27208e..63b53f7 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -67,10 +67,6 @@
#define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
ETH_TXQ_FLAGS_NOOFFLOADS)
-#ifdef RTE_MACHINE_CPUFLAG_SSSE3
-static int use_simple_rxtx;
-#endif
-
static void
vq_ring_free_chain(struct virtqueue *vq, uint16_t desc_idx)
{
@@ -333,6 +329,7 @@ virtio_dev_rxtx_start(struct rte_eth_dev *dev)
*/
uint16_t i;
uint16_t desc_idx;
+ struct virtio_hw *hw = dev->data->dev_private;
PMD_INIT_FUNC_TRACE();
@@ -353,8 +350,7 @@ virtio_dev_rxtx_start(struct rte_eth_dev *dev)
nbufs = 0;
error = ENOSPC;
-#ifdef RTE_MACHINE_CPUFLAG_SSSE3
- if (use_simple_rxtx) {
+ if (hw->use_simple_rxtx) {
for (desc_idx = 0; desc_idx < vq->vq_nentries;
desc_idx++) {
vq->vq_ring.avail->ring[desc_idx] = desc_idx;
@@ -362,7 +358,7 @@ virtio_dev_rxtx_start(struct rte_eth_dev *dev)
VRING_DESC_F_WRITE;
}
}
-#endif
+
memset(&rxvq->fake_mbuf, 0, sizeof(rxvq->fake_mbuf));
for (desc_idx = 0; desc_idx < RTE_PMD_VIRTIO_RX_MAX_BURST;
desc_idx++) {
@@ -378,12 +374,11 @@ virtio_dev_rxtx_start(struct rte_eth_dev *dev)
/**
* Enqueue allocated buffers*
***/
-#ifdef RTE_MACHINE_CPUFLAG_SSSE3
- if (use_simple_rxtx)
+ if (hw->use_simple_rxtx)
error =
virtqueue_enqueue_recv_refill_simple(vq, m);
else
-#endif
error = virtqueue_enqueue_recv_refill(vq, m);
+
if (error) {
rte_pktmbuf_free(m);
break;
@@ -404,8 +399,7 @@ virtio_dev_rxtx_start(struct rte_eth_dev *dev)
struct virtqueue *vq = txvq->vq;
virtio_dev_vring_start(vq);
-#ifdef RTE_MACHINE_CPUFLAG_SSSE3
- if (use_simple_rxtx) {
+ if (hw->use_simple_rxtx) {
uint16_t mid_idx = vq->vq_nentries >> 1;
for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
@@ -426,7 +420,7 @@ virtio_dev_rxtx_start(struct rte_eth_dev *dev)
desc_idx++)
vq->vq_ring.avail->ring[desc_idx] = desc_idx;
}
-#endif
+
VIRTQUEUE_DUMP(vq);
}
}
@@ -456,9 +450,7 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
dev->data->rx_queues[queue_idx] = rxvq;
-#ifdef RTE_MACHINE_CPUFLAG_SSSE3
virtio_rxq_vec_setup(rxvq);
-#endif
return 0;
}
@@ -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;
