[net-next-2.6 PATCH 1/3] ixgbe: Fix DMA mapping/unmapping issues when HWRSC is enabled on IOMMU enabled kernels

[net-next-2.6 PATCH 1/3] ixgbe: Fix DMA mapping/unmapping issues when HWRSC is enabled on IOMMU enabled kernels

[Jeff Kirsher]

From: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>

Work around 82599 HW issue when HWRSC is enabled on IOMMU enabled
kernels. 82599 HW is updating the header information after setting the
descriptor to done, resulting DMA mapping/unmapping issues on IOMMU
enabled systems. To work around the issue delay unmapping of first packet
that carries the header information until end of packet is reached.

Signed-off-by: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_main.c |   32 +++++++++++++++++++++++++++++---
 1 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 3308790..4a01022 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -818,6 +818,12 @@ static inline struct sk_buff *ixgbe_transform_rsc_queue(struct sk_buff *skb,
 	return skb;
 }
 
+struct ixgbe_rsc_cb {
+	dma_addr_t dma;
+};
+
+#define IXGBE_RSC_CB(skb) ((struct ixgbe_rsc_cb *)(skb)->cb)
+
 static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
                                struct ixgbe_ring *rx_ring,
                                int *work_done, int work_to_do)
@@ -867,9 +873,21 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 		rx_buffer_info->skb = NULL;
 
 		if (rx_buffer_info->dma) {
-			pci_unmap_single(pdev, rx_buffer_info->dma,
-			                 rx_ring->rx_buf_len,
-			                 PCI_DMA_FROMDEVICE);
+			if ((adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
+			    (!(staterr & IXGBE_RXD_STAT_EOP)) &&
+				 (!(skb->prev)))
+				/*
+				 * When HWRSC is enabled, delay unmapping
+				 * of the first packet. It carries the
+				 * header information, HW may still
+				 * access the header after the writeback.
+				 * Only unmap it when EOP is reached
+				 */
+				IXGBE_RSC_CB(skb)->dma = rx_buffer_info->dma;
+			else
+				pci_unmap_single(pdev, rx_buffer_info->dma,
+				                 rx_ring->rx_buf_len,
+				                 PCI_DMA_FROMDEVICE);
 			rx_buffer_info->dma = 0;
 			skb_put(skb, len);
 		}
@@ -917,6 +935,10 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 			if (skb->prev)
 				skb = ixgbe_transform_rsc_queue(skb, &(rx_ring->rsc_count));
 			if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
+				if (IXGBE_RSC_CB(skb)->dma)
+					pci_unmap_single(pdev, IXGBE_RSC_CB(skb)->dma,
+					                 rx_ring->rx_buf_len,
+					                 PCI_DMA_FROMDEVICE);
 				if (rx_ring->flags & IXGBE_RING_RX_PS_ENABLED)
 					rx_ring->rsc_count += skb_shinfo(skb)->nr_frags;
 				else
@@ -3104,6 +3126,10 @@ static void ixgbe_clean_rx_ring(struct ixgbe_adapter *adapter,
 			rx_buffer_info->skb = NULL;
 			do {
 				struct sk_buff *this = skb;
+				if (IXGBE_RSC_CB(this)->dma)
+					pci_unmap_single(pdev, IXGBE_RSC_CB(this)->dma,
+					                 rx_ring->rx_buf_len,
+					                 PCI_DMA_FROMDEVICE);
 				skb = skb->prev;
 				dev_kfree_skb(this);
 			} while (skb);

[net-next-2.6 PATCH 2/3] ixgbe: do not stop tx queues in ixgbe_set_tso

[Jeff Kirsher]

From: John Fastabend <john.r.fastabend@intel.com>

Disabling TSO can cause the dev_watchdog timer to be triggered because
when TSO is disabled netif_tx_stop_all_queues is called.  If the watchdog
timer fires while the queues are stopped and traffic has not recently been
sent on a paticular queue this is falsly identified as a hang and
ndo_tx_timeout() is called.  This is ocossionally seen during testing.

This removes the netif_tx_stop_all_queues() it is not needed.  The scheduler
submits skb's with dev_hard_start_xmit(), this checks if netif_needs_gso and
if so it calls dev_gso_segment.  Disabling TSO will cause dev_hard_start_xmit()
to do the gso processing.   However ixgbe does not use the features flags to
determine if it needs to use tso or not instead it uses skb->gso_size so
ixgbe will process these frames correctly regardless of the netdev features
flag.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_ethtool.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethtool.c b/drivers/net/ixgbe/ixgbe_ethtool.c
index 0d23434..7949a44 100644
--- a/drivers/net/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ixgbe/ixgbe_ethtool.c
@@ -441,10 +441,8 @@ static int ixgbe_set_tso(struct net_device *netdev, u32 data)
 		netdev->features |= NETIF_F_TSO;
 		netdev->features |= NETIF_F_TSO6;
 	} else {
-		netif_tx_stop_all_queues(netdev);
 		netdev->features &= ~NETIF_F_TSO;
 		netdev->features &= ~NETIF_F_TSO6;
-		netif_tx_start_all_queues(netdev);
 	}
 	return 0;
 }

[net-next-2.6 PATCH 3/3] ixgbe: Do not allocate too many netdev txqueues

[Jeff Kirsher]

From: John Fastabend <john.r.fastabend@intel.com>

Instead of allocating 128 struct netdev_queue per device, use the
minimum value between 128 and the number of possible txq's, to
reduce ram usage and "tc -s -d class shod dev .." output.

This patch fixes Eric Dumazet's patch to set the TX queues to
the correct minimum.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_main.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 4a01022..a961da2 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -5996,6 +5996,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 	const struct ixgbe_info *ii = ixgbe_info_tbl[ent->driver_data];
 	static int cards_found;
 	int i, err, pci_using_dac;
+	unsigned int indices = num_possible_cpus();
 #ifdef IXGBE_FCOE
 	u16 device_caps;
 #endif
@@ -6034,7 +6035,18 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 	pci_set_master(pdev);
 	pci_save_state(pdev);
 
-	netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), MAX_TX_QUEUES);
+	if (ii->mac == ixgbe_mac_82598EB)
+		indices = min_t(unsigned int, indices, IXGBE_MAX_RSS_INDICES);
+	else
+		indices = min_t(unsigned int, indices, IXGBE_MAX_FDIR_INDICES);
+
+	indices = max_t(unsigned int, indices, IXGBE_MAX_DCB_INDICES);
+#ifdef IXGBE_FCOE
+	indices += min_t(unsigned int, num_possible_cpus(),
+			 IXGBE_MAX_FCOE_INDICES);
+#endif
+	indices = min_t(unsigned int, indices, MAX_TX_QUEUES);
+	netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), indices);
 	if (!netdev) {
 		err = -ENOMEM;
 		goto err_alloc_etherdev;

Re: [net-next-2.6 PATCH 1/3] ixgbe: Fix DMA mapping/unmapping issues when HWRSC is enabled on IOMMU enabled kernels

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 26 Feb 2010 01:14:37 -0800

> From: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>
> 
> Work around 82599 HW issue when HWRSC is enabled on IOMMU enabled
> kernels. 82599 HW is updating the header information after setting the
> descriptor to done, resulting DMA mapping/unmapping issues on IOMMU
> enabled systems. To work around the issue delay unmapping of first packet
> that carries the header information until end of packet is reached.
> 
> Signed-off-by: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied.

Re: [net-next-2.6 PATCH 2/3] ixgbe: do not stop tx queues in ixgbe_set_tso

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 26 Feb 2010 01:14:58 -0800

> From: John Fastabend <john.r.fastabend@intel.com>
> 
> Disabling TSO can cause the dev_watchdog timer to be triggered because
> when TSO is disabled netif_tx_stop_all_queues is called.  If the watchdog
> timer fires while the queues are stopped and traffic has not recently been
> sent on a paticular queue this is falsly identified as a hang and
> ndo_tx_timeout() is called.  This is ocossionally seen during testing.
> 
> This removes the netif_tx_stop_all_queues() it is not needed.  The scheduler
> submits skb's with dev_hard_start_xmit(), this checks if netif_needs_gso and
> if so it calls dev_gso_segment.  Disabling TSO will cause dev_hard_start_xmit()
> to do the gso processing.   However ixgbe does not use the features flags to
> determine if it needs to use tso or not instead it uses skb->gso_size so
> ixgbe will process these frames correctly regardless of the netdev features
> flag.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied.

Re: [net-next-2.6 PATCH 3/3] ixgbe: Do not allocate too many netdev txqueues

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 26 Feb 2010 01:15:21 -0800

> From: John Fastabend <john.r.fastabend@intel.com>
> 
> Instead of allocating 128 struct netdev_queue per device, use the
> minimum value between 128 and the number of possible txq's, to
> reduce ram usage and "tc -s -d class shod dev .." output.
> 
> This patch fixes Eric Dumazet's patch to set the TX queues to
> the correct minimum.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied.

Re: [net-next-2.6 PATCH 3/3] ixgbe: Do not allocate too many netdev txqueues

[Eric Dumazet]

Le vendredi 26 février 2010 à 01:15 -0800, Jeff Kirsher a écrit :
> From: John Fastabend <john.r.fastabend@intel.com>
> 
> Instead of allocating 128 struct netdev_queue per device, use the
> minimum value between 128 and the number of possible txq's, to
> reduce ram usage and "tc -s -d class shod dev .." output.
> 
> This patch fixes Eric Dumazet's patch to set the TX queues to
> the correct minimum.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> 
>  drivers/net/ixgbe/ixgbe_main.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index 4a01022..a961da2 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -5996,6 +5996,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  	const struct ixgbe_info *ii = ixgbe_info_tbl[ent->driver_data];
>  	static int cards_found;
>  	int i, err, pci_using_dac;
> +	unsigned int indices = num_possible_cpus();
>  #ifdef IXGBE_FCOE
>  	u16 device_caps;
>  #endif
> @@ -6034,7 +6035,18 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  	pci_set_master(pdev);
>  	pci_save_state(pdev);
>  
> -	netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), MAX_TX_QUEUES);
> +	if (ii->mac == ixgbe_mac_82598EB)
> +		indices = min_t(unsigned int, indices, IXGBE_MAX_RSS_INDICES);
> +	else
> +		indices = min_t(unsigned int, indices, IXGBE_MAX_FDIR_INDICES);
> +
> +	indices = max_t(unsigned int, indices, IXGBE_MAX_DCB_INDICES);
> +#ifdef IXGBE_FCOE
> +	indices += min_t(unsigned int, num_possible_cpus(),
> +			 IXGBE_MAX_FCOE_INDICES);
> +#endif
> +	indices = min_t(unsigned int, indices, MAX_TX_QUEUES);
> +	netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), indices);
>  	if (!netdev) {
>  		err = -ENOMEM;
>  		goto err_alloc_etherdev;
> 

Thanks Jeff, but what is the reason for limiting to MAX_TX_QUEUES ?
Is it a hardware issue ?

Re: [net-next-2.6 PATCH 3/3] ixgbe: Do not allocate too many netdev txqueues

[Peter P Waskiewicz Jr]

On Fri, 2010-02-26 at 06:04 -0800, Eric Dumazet wrote:
> Le vendredi 26 février 2010 à 01:15 -0800, Jeff Kirsher a écrit :
> > From: John Fastabend <john.r.fastabend@intel.com>
> > 
> > Instead of allocating 128 struct netdev_queue per device, use the
> > minimum value between 128 and the number of possible txq's, to
> > reduce ram usage and "tc -s -d class shod dev .." output.
> > 
> > This patch fixes Eric Dumazet's patch to set the TX queues to
> > the correct minimum.
> > 
> > Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > ---
> > 
> >  drivers/net/ixgbe/ixgbe_main.c |   14 +++++++++++++-
> >  1 files changed, 13 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> > index 4a01022..a961da2 100644
> > --- a/drivers/net/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ixgbe/ixgbe_main.c
> > @@ -5996,6 +5996,7 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
> >  	const struct ixgbe_info *ii = ixgbe_info_tbl[ent->driver_data];
> >  	static int cards_found;
> >  	int i, err, pci_using_dac;
> > +	unsigned int indices = num_possible_cpus();
> >  #ifdef IXGBE_FCOE
> >  	u16 device_caps;
> >  #endif
> > @@ -6034,7 +6035,18 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
> >  	pci_set_master(pdev);
> >  	pci_save_state(pdev);
> >  
> > -	netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), MAX_TX_QUEUES);
> > +	if (ii->mac == ixgbe_mac_82598EB)
> > +		indices = min_t(unsigned int, indices, IXGBE_MAX_RSS_INDICES);
> > +	else
> > +		indices = min_t(unsigned int, indices, IXGBE_MAX_FDIR_INDICES);
> > +
> > +	indices = max_t(unsigned int, indices, IXGBE_MAX_DCB_INDICES);
> > +#ifdef IXGBE_FCOE
> > +	indices += min_t(unsigned int, num_possible_cpus(),
> > +			 IXGBE_MAX_FCOE_INDICES);
> > +#endif
> > +	indices = min_t(unsigned int, indices, MAX_TX_QUEUES);
> > +	netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), indices);
> >  	if (!netdev) {
> >  		err = -ENOMEM;
> >  		goto err_alloc_etherdev;
> > 
> 
> Thanks Jeff, but what is the reason for limiting to MAX_TX_QUEUES ?
> Is it a hardware issue ?
> 

MAX_TX_QUEUES is 128, which is the maximum the 82599 device supports in
hardware (82598 supports 32 Tx queues).  I'm not sure why you'd ever
want to have more Tx queues than what you have in the network device.
Perhaps I don't understand the question?

Cheers,
-PJ

Re: [net-next-2.6 PATCH 3/3] ixgbe: Do not allocate too many netdev txqueues

[Eric Dumazet]

Le samedi 27 février 2010 à 17:02 -0800, Peter P Waskiewicz Jr a écrit :
> On Fri, 2010-02-26 at 06:04 -0800, Eric Dumazet wrote:
> > Le vendredi 26 février 2010 à 01:15 -0800, Jeff Kirsher a écrit :
> > > +	if (ii->mac == ixgbe_mac_82598EB)
> > > +		indices = min_t(unsigned int, indices, IXGBE_MAX_RSS_INDICES);
> > > +	else
> > > +		indices = min_t(unsigned int, indices, IXGBE_MAX_FDIR_INDICES);
> > > +
> > > +	indices = max_t(unsigned int, indices, IXGBE_MAX_DCB_INDICES);
> > > +#ifdef IXGBE_FCOE
> > > +	indices += min_t(unsigned int, num_possible_cpus(),
> > > +			 IXGBE_MAX_FCOE_INDICES);
> > > +#endif
> > > +	indices = min_t(unsigned int, indices, MAX_TX_QUEUES);
> > > +	netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), indices);
> > >  	if (!netdev) {
> > >  		err = -ENOMEM;
> > >  		goto err_alloc_etherdev;
> > > 
> > 
> > Thanks Jeff, but what is the reason for limiting to MAX_TX_QUEUES ?
> > Is it a hardware issue ?
> > 
> 
> MAX_TX_QUEUES is 128, which is the maximum the 82599 device supports in
> hardware (82598 supports 32 Tx queues).  I'm not sure why you'd ever
> want to have more Tx queues than what you have in the network device.

I was not sure MAX_TX_QUEUES capping was still necessary after the
block :

if (ii->mac == ixgbe_mac_82598EB)
          indices = min_t(unsigned int, indices, IXGBE_MAX_RSS_INDICES);
else
          indices = min_t(unsigned int, indices,
IXGBE_MAX_FDIR_INDICES);

indices = max_t(unsigned int, indices, IXGBE_MAX_DCB_INDICES);
#ifdef IXGBE_FCOE
	indices += min_t(unsigned int, num_possible_cpus(),
                    IXGBE_MAX_FCOE_INDICES);
#endif

So I asked to be sure that MAX_TX_QUEUES was not a leftover from the
previous default allocation.

Thanks

Re: [net-next-2.6 PATCH 3/3] ixgbe: Do not allocate too many netdev txqueues

[Peter P Waskiewicz Jr]

On Sat, 2010-02-27 at 20:57 -0700, Eric Dumazet wrote:
> Le samedi 27 février 2010 à 17:02 -0800, Peter P Waskiewicz Jr a écrit :
> > On Fri, 2010-02-26 at 06:04 -0800, Eric Dumazet wrote:
> > > Le vendredi 26 février 2010 à 01:15 -0800, Jeff Kirsher a écrit :
> > > > +	if (ii->mac == ixgbe_mac_82598EB)
> > > > +		indices = min_t(unsigned int, indices, IXGBE_MAX_RSS_INDICES);
> > > > +	else
> > > > +		indices = min_t(unsigned int, indices, IXGBE_MAX_FDIR_INDICES);
> > > > +
> > > > +	indices = max_t(unsigned int, indices, IXGBE_MAX_DCB_INDICES);
> > > > +#ifdef IXGBE_FCOE
> > > > +	indices += min_t(unsigned int, num_possible_cpus(),
> > > > +			 IXGBE_MAX_FCOE_INDICES);
> > > > +#endif
> > > > +	indices = min_t(unsigned int, indices, MAX_TX_QUEUES);
> > > > +	netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), indices);
> > > >  	if (!netdev) {
> > > >  		err = -ENOMEM;
> > > >  		goto err_alloc_etherdev;
> > > > 
> > > 
> > > Thanks Jeff, but what is the reason for limiting to MAX_TX_QUEUES ?
> > > Is it a hardware issue ?
> > > 
> > 
> > MAX_TX_QUEUES is 128, which is the maximum the 82599 device supports in
> > hardware (82598 supports 32 Tx queues).  I'm not sure why you'd ever
> > want to have more Tx queues than what you have in the network device.
> 
> I was not sure MAX_TX_QUEUES capping was still necessary after the
> block :
> 
> if (ii->mac == ixgbe_mac_82598EB)
>           indices = min_t(unsigned int, indices, IXGBE_MAX_RSS_INDICES);
> else
>           indices = min_t(unsigned int, indices,
> IXGBE_MAX_FDIR_INDICES);
> 
> indices = max_t(unsigned int, indices, IXGBE_MAX_DCB_INDICES);
> #ifdef IXGBE_FCOE
> 	indices += min_t(unsigned int, num_possible_cpus(),
>                     IXGBE_MAX_FCOE_INDICES);
> #endif
> 
> So I asked to be sure that MAX_TX_QUEUES was not a leftover from the
> previous default allocation.
> 
> Thanks

I see what you're getting at now.  The most we could have from this
codepath is 72 indices for 82599, and 24 for 82598, so yeah, this is
probably unneeded.

We can get the patch cleaned up.

Cheers,
-PJ

Re: [net-next-2.6 PATCH 3/3] ixgbe: Do not allocate too many netdev txqueues

[John Fastabend]

Waskiewicz Jr, Peter P wrote:
> On Sat, 2010-02-27 at 20:57 -0700, Eric Dumazet wrote:
>   
>> Le samedi 27 février 2010 à 17:02 -0800, Peter P Waskiewicz Jr a écrit :
>>     
>>> On Fri, 2010-02-26 at 06:04 -0800, Eric Dumazet wrote:
>>>       
>>>> Le vendredi 26 février 2010 à 01:15 -0800, Jeff Kirsher a écrit :
>>>>         
>>>>> +	if (ii->mac == ixgbe_mac_82598EB)
>>>>> +		indices = min_t(unsigned int, indices, IXGBE_MAX_RSS_INDICES);
>>>>> +	else
>>>>> +		indices = min_t(unsigned int, indices, IXGBE_MAX_FDIR_INDICES);
>>>>> +
>>>>> +	indices = max_t(unsigned int, indices, IXGBE_MAX_DCB_INDICES);
>>>>> +#ifdef IXGBE_FCOE
>>>>> +	indices += min_t(unsigned int, num_possible_cpus(),
>>>>> +			 IXGBE_MAX_FCOE_INDICES);
>>>>> +#endif
>>>>> +	indices = min_t(unsigned int, indices, MAX_TX_QUEUES);
>>>>> +	netdev = alloc_etherdev_mq(sizeof(struct ixgbe_adapter), indices);
>>>>>  	if (!netdev) {
>>>>>  		err = -ENOMEM;
>>>>>  		goto err_alloc_etherdev;
>>>>>
>>>>>           
>>>> Thanks Jeff, but what is the reason for limiting to MAX_TX_QUEUES ?
>>>> Is it a hardware issue ?
>>>>
>>>>         
>>> MAX_TX_QUEUES is 128, which is the maximum the 82599 device supports in
>>> hardware (82598 supports 32 Tx queues).  I'm not sure why you'd ever
>>> want to have more Tx queues than what you have in the network device.
>>>       
>> I was not sure MAX_TX_QUEUES capping was still necessary after the
>> block :
>>
>> if (ii->mac == ixgbe_mac_82598EB)
>>           indices = min_t(unsigned int, indices, IXGBE_MAX_RSS_INDICES);
>> else
>>           indices = min_t(unsigned int, indices,
>> IXGBE_MAX_FDIR_INDICES);
>>
>> indices = max_t(unsigned int, indices, IXGBE_MAX_DCB_INDICES);
>> #ifdef IXGBE_FCOE
>> 	indices += min_t(unsigned int, num_possible_cpus(),
>>                     IXGBE_MAX_FCOE_INDICES);
>> #endif
>>
>> So I asked to be sure that MAX_TX_QUEUES was not a leftover from the
>> previous default allocation.
>>
>> Thanks
>>     
>
> I see what you're getting at now.  The most we could have from this
> codepath is 72 indices for 82599, and 24 for 82598, so yeah, this is
> probably unneeded.
>
> We can get the patch cleaned up.
>
> Cheers,
> -PJ
>
>   
Thanks for catching this.  I'll submit another patch to clean this up.  
Additionally, we can probably remove MAX_TX_QUEUES all together and use 
the above values instead.  No reason to have a tx_ring array larger then 
we are ever going to use.

thanks
john.

Re: [net-next-2.6 PATCH 1/3] ixgbe: Fix DMA mapping/unmapping issues when HWRSC is enabled on IOMMU enabled kernels

[Simon Horman]

On Fri, Feb 26, 2010 at 01:14:37AM -0800, Jeff Kirsher wrote:
> From: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>
> 
> Work around 82599 HW issue when HWRSC is enabled on IOMMU enabled
> kernels. 82599 HW is updating the header information after setting the
> descriptor to done, resulting DMA mapping/unmapping issues on IOMMU
> enabled systems. To work around the issue delay unmapping of first packet
> that carries the header information until end of packet is reached.
> 
> Signed-off-by: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> 
>  drivers/net/ixgbe/ixgbe_main.c |   32 +++++++++++++++++++++++++++++---
>  1 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index 3308790..4a01022 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -818,6 +818,12 @@ static inline struct sk_buff *ixgbe_transform_rsc_queue(struct sk_buff *skb,
>  	return skb;
>  }
>  
> +struct ixgbe_rsc_cb {
> +	dma_addr_t dma;
> +};
> +
> +#define IXGBE_RSC_CB(skb) ((struct ixgbe_rsc_cb *)(skb)->cb)
> +
>  static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
>                                 struct ixgbe_ring *rx_ring,
>                                 int *work_done, int work_to_do)
> @@ -867,9 +873,21 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
>  		rx_buffer_info->skb = NULL;
>  
>  		if (rx_buffer_info->dma) {
> -			pci_unmap_single(pdev, rx_buffer_info->dma,
> -			                 rx_ring->rx_buf_len,
> -			                 PCI_DMA_FROMDEVICE);
> +			if ((adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
> +			    (!(staterr & IXGBE_RXD_STAT_EOP)) &&
> +				 (!(skb->prev)))
> +				/*
> +				 * When HWRSC is enabled, delay unmapping
> +				 * of the first packet. It carries the
> +				 * header information, HW may still
> +				 * access the header after the writeback.
> +				 * Only unmap it when EOP is reached
> +				 */
> +				IXGBE_RSC_CB(skb)->dma = rx_buffer_info->dma;
> +			else
> +				pci_unmap_single(pdev, rx_buffer_info->dma,
> +				                 rx_ring->rx_buf_len,
> +				                 PCI_DMA_FROMDEVICE);
>  			rx_buffer_info->dma = 0;
>  			skb_put(skb, len);
>  		}
> @@ -917,6 +935,10 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
>  			if (skb->prev)
>  				skb = ixgbe_transform_rsc_queue(skb, &(rx_ring->rsc_count));
>  			if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
> +				if (IXGBE_RSC_CB(skb)->dma)
> +					pci_unmap_single(pdev, IXGBE_RSC_CB(skb)->dma,
> +					                 rx_ring->rx_buf_len,
> +					                 PCI_DMA_FROMDEVICE);

Does IXGBE_RSC_CB(skb)->dma need to be set to NULL here
to avoid a double-free in ixgbe_clean_rx_ring() ?

>  				if (rx_ring->flags & IXGBE_RING_RX_PS_ENABLED)
>  					rx_ring->rsc_count += skb_shinfo(skb)->nr_frags;
>  				else
> @@ -3104,6 +3126,10 @@ static void ixgbe_clean_rx_ring(struct ixgbe_adapter *adapter,
>  			rx_buffer_info->skb = NULL;
>  			do {
>  				struct sk_buff *this = skb;
> +				if (IXGBE_RSC_CB(this)->dma)
> +					pci_unmap_single(pdev, IXGBE_RSC_CB(this)->dma,
> +					                 rx_ring->rx_buf_len,
> +					                 PCI_DMA_FROMDEVICE);
>  				skb = skb->prev;
>  				dev_kfree_skb(this);
>  			} while (skb);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [net-next-2.6 PATCH 1/3] ixgbe: Fix DMA mapping/unmapping issues when HWRSC is enabled on IOMMU enabled kernels

[Chilakala, Mallikarjuna]

>-----Original Message-----
>From: Simon Horman [mailto:horms@verge.net.au]
>Sent: Monday, March 01, 2010 4:29 PM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
>Chilakala, Mallikarjuna
>Subject: Re: [net-next-2.6 PATCH 1/3] ixgbe: Fix DMA mapping/unmapping
>issues when HWRSC is enabled on IOMMU enabled kernels
>
>On Fri, Feb 26, 2010 at 01:14:37AM -0800, Jeff Kirsher wrote:
>> From: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>
>>
>> Work around 82599 HW issue when HWRSC is enabled on IOMMU enabled
>> kernels. 82599 HW is updating the header information after setting the
>> descriptor to done, resulting DMA mapping/unmapping issues on IOMMU
>> enabled systems. To work around the issue delay unmapping of first packet
>> that carries the header information until end of packet is reached.
>>
>> Signed-off-by: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>>
>>  drivers/net/ixgbe/ixgbe_main.c |   32 +++++++++++++++++++++++++++++---
>>  1 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_main.c
>b/drivers/net/ixgbe/ixgbe_main.c
>> index 3308790..4a01022 100644
>> --- a/drivers/net/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ixgbe/ixgbe_main.c
>> @@ -818,6 +818,12 @@ static inline struct sk_buff
>*ixgbe_transform_rsc_queue(struct sk_buff *skb,
>>  	return skb;
>>  }
>>
>> +struct ixgbe_rsc_cb {
>> +	dma_addr_t dma;
>> +};
>> +
>> +#define IXGBE_RSC_CB(skb) ((struct ixgbe_rsc_cb *)(skb)->cb)
>> +
>>  static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
>>                                 struct ixgbe_ring *rx_ring,
>>                                 int *work_done, int work_to_do)
>> @@ -867,9 +873,21 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector
>*q_vector,
>>  		rx_buffer_info->skb = NULL;
>>
>>  		if (rx_buffer_info->dma) {
>> -			pci_unmap_single(pdev, rx_buffer_info->dma,
>> -			                 rx_ring->rx_buf_len,
>> -			                 PCI_DMA_FROMDEVICE);
>> +			if ((adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
>> +			    (!(staterr & IXGBE_RXD_STAT_EOP)) &&
>> +				 (!(skb->prev)))
>> +				/*
>> +				 * When HWRSC is enabled, delay unmapping
>> +				 * of the first packet. It carries the
>> +				 * header information, HW may still
>> +				 * access the header after the writeback.
>> +				 * Only unmap it when EOP is reached
>> +				 */
>> +				IXGBE_RSC_CB(skb)->dma = rx_buffer_info->dma;
>> +			else
>> +				pci_unmap_single(pdev, rx_buffer_info->dma,
>> +				                 rx_ring->rx_buf_len,
>> +				                 PCI_DMA_FROMDEVICE);
>>  			rx_buffer_info->dma = 0;
>>  			skb_put(skb, len);
>>  		}
>> @@ -917,6 +935,10 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector
>*q_vector,
>>  			if (skb->prev)
>>  				skb = ixgbe_transform_rsc_queue(skb, &(rx_ring-
>>rsc_count));
>>  			if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
>> +				if (IXGBE_RSC_CB(skb)->dma)
>> +					pci_unmap_single(pdev, IXGBE_RSC_CB(skb)->dma,
>> +					                 rx_ring->rx_buf_len,
>> +					                 PCI_DMA_FROMDEVICE);
>

>Does IXGBE_RSC_CB(skb)->dma need to be set to NULL here
>to avoid a double-free in ixgbe_clean_rx_ring() ?

It shouldn't happen, but it is good to set it to NULL.
I'll send out a patch to fix this issue.
Thanks for your review Horman.

>
>>  				if (rx_ring->flags & IXGBE_RING_RX_PS_ENABLED)
>>  					rx_ring->rsc_count += skb_shinfo(skb)-
>>nr_frags;
>>  				else
>> @@ -3104,6 +3126,10 @@ static void ixgbe_clean_rx_ring(struct
>ixgbe_adapter *adapter,
>>  			rx_buffer_info->skb = NULL;
>>  			do {
>>  				struct sk_buff *this = skb;
>> +				if (IXGBE_RSC_CB(this)->dma)
>> +					pci_unmap_single(pdev, IXGBE_RSC_CB(this)-
>>dma,
>> +					                 rx_ring->rx_buf_len,
>> +					                 PCI_DMA_FROMDEVICE);
>>  				skb = skb->prev;
>>  				dev_kfree_skb(this);
>>  			} while (skb);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html