ixgbe: normalize frag_list usage

ixgbe: normalize frag_list usage

[David Miller]

Signed-off-by: David S. Miller <davem@davemloft.net>
---

Basically I want to get the whole tree to consistently use
the convention that the frag list is built as packets arrive
by using "skb->prev" of the head skb to track the tail member
of the frag list.

This will allow me to do something like:

struct sk_buff {
       union {
		struct {
			struct sk_buff *next;
			struct sk_buff *prev;
		};
		struct {
			struct sk_buff *frag_next;
			struct sk_buff *frag_tail_tracker;
		};
	}
 ...
}

And have all frag_list code use these members consistently.

While doing this patch I noticed that there are some left-over bits in
the IXGBEVF driver that builds these IXGBE style (before my patch)
skb->prev RSC chains.

But nothing other than the RX ring shutdown code processes them, so
likely if they are created they will leak or cause some other kind of
problem.

Please take a look, thanks.

diff --git a/drivers/net/ixgbe/ixgbe.h b/drivers/net/ixgbe/ixgbe.h
index 5cebc37..434c9fa 100644
--- a/drivers/net/ixgbe/ixgbe.h
+++ b/drivers/net/ixgbe/ixgbe.h
@@ -181,6 +181,7 @@ struct ixgbe_ring {
 	struct ixgbe_queue_stats stats;
 	unsigned long reinit_state;
 	int numa_node;
+	struct sk_buff *rsc_skb;	/* RSC packet being built */
 	u64 rsc_count;			/* stat for coalesced packets */
 	u64 rsc_flush;			/* stats for flushed packets */
 	u32 restart_queue;		/* track tx queue restarts */
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index c35e13c..43987a4 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -1130,36 +1130,6 @@ static inline u32 ixgbe_get_rsc_count(union ixgbe_adv_rx_desc *rx_desc)
 		IXGBE_RXDADV_RSCCNT_SHIFT;
 }
 
-/**
- * ixgbe_transform_rsc_queue - change rsc queue into a full packet
- * @skb: pointer to the last skb in the rsc queue
- * @count: pointer to number of packets coalesced in this context
- *
- * This function changes a queue full of hw rsc buffers into a completed
- * packet.  It uses the ->prev pointers to find the first packet and then
- * turns it into the frag list owner.
- **/
-static inline struct sk_buff *ixgbe_transform_rsc_queue(struct sk_buff *skb,
-							u64 *count)
-{
-	unsigned int frag_list_size = 0;
-
-	while (skb->prev) {
-		struct sk_buff *prev = skb->prev;
-		frag_list_size += skb->len;
-		skb->prev = NULL;
-		skb = prev;
-		*count += 1;
-	}
-
-	skb_shinfo(skb)->frag_list = skb->next;
-	skb->next = NULL;
-	skb->len += frag_list_size;
-	skb->data_len += frag_list_size;
-	skb->truesize += frag_list_size;
-	return skb;
-}
-
 struct ixgbe_rsc_cb {
 	dma_addr_t dma;
 	bool delay_unmap;
@@ -1216,10 +1186,13 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 		prefetch(skb->data);
 		rx_buffer_info->skb = NULL;
 
+		if (!(staterr & IXGBE_RXD_STAT_EOP)) {
+			rx_ring->rsc_skb = skb;
+			rx_ring->rsc_count++;
+		}
 		if (rx_buffer_info->dma) {
 			if ((adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
-			    (!(staterr & IXGBE_RXD_STAT_EOP)) &&
-				 (!(skb->prev))) {
+			    rx_ring->rsc_skb == skb) {
 				/*
 				 * When HWRSC is enabled, delay unmapping
 				 * of the first packet. It carries the
@@ -1279,9 +1252,12 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 		}
 
 		if (staterr & IXGBE_RXD_STAT_EOP) {
-			if (skb->prev)
-				skb = ixgbe_transform_rsc_queue(skb,
-								&(rx_ring->rsc_count));
+			if (rx_ring->rsc_skb) {
+				skb = rx_ring->rsc_skb;
+				rx_ring->rsc_skb = NULL;
+
+				skb->prev = NULL;
+			}
 			if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
 				if (IXGBE_RSC_CB(skb)->delay_unmap) {
 					dma_unmap_single(&pdev->dev,
@@ -1307,8 +1283,22 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 				next_buffer->skb = skb;
 				next_buffer->dma = 0;
 			} else {
-				skb->next = next_buffer->skb;
-				skb->next->prev = skb;
+				struct sk_buff *head = rx_ring->rsc_skb;
+				struct sk_buff *next = next_buffer->skb;
+
+				if (!skb_shinfo(head)->frag_list) {
+					skb_shinfo(head)->frag_list = next;
+				} else {
+					head->prev->next = next;
+				}
+
+				/* ->prev tracks the last skb in the frag_list */
+				head->prev = next;
+				rx_ring->rsc_count++;
+
+				head->len += next->len;
+				head->data_len += next->len;
+				head->truesize += next->len;
 			}
 			rx_ring->non_eop_descs++;
 			goto next_desc;

RE: ixgbe: normalize frag_list usage

[Rose, Gregory V]

RSC isn't actually supported in virtual functions or even when SR-IOV mode is enabled.  Any left over bits are just cruft from when the vf driver was ported from the pf driver.

I'll have a look at it.

- Gerg

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of David Miller
> Sent: Sunday, October 03, 2010 11:54 PM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; Allan, Bruce W; netdev@vger.kernel.org
> Subject: ixgbe: normalize frag_list usage
> 
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
> 
> Basically I want to get the whole tree to consistently use
> the convention that the frag list is built as packets arrive
> by using "skb->prev" of the head skb to track the tail member
> of the frag list.
> 
> This will allow me to do something like:
> 
> struct sk_buff {
>        union {
> 		struct {
> 			struct sk_buff *next;
> 			struct sk_buff *prev;
> 		};
> 		struct {
> 			struct sk_buff *frag_next;
> 			struct sk_buff *frag_tail_tracker;
> 		};
> 	}
>  ...
> }
> 
> And have all frag_list code use these members consistently.
> 
> While doing this patch I noticed that there are some left-over bits in
> the IXGBEVF driver that builds these IXGBE style (before my patch)
> skb->prev RSC chains.
> 
> But nothing other than the RX ring shutdown code processes them, so
> likely if they are created they will leak or cause some other kind of
> problem.
> 
> Please take a look, thanks.
> 
> diff --git a/drivers/net/ixgbe/ixgbe.h b/drivers/net/ixgbe/ixgbe.h
> index 5cebc37..434c9fa 100644
> --- a/drivers/net/ixgbe/ixgbe.h
> +++ b/drivers/net/ixgbe/ixgbe.h
> @@ -181,6 +181,7 @@ struct ixgbe_ring {
>  	struct ixgbe_queue_stats stats;
>  	unsigned long reinit_state;
>  	int numa_node;
> +	struct sk_buff *rsc_skb;	/* RSC packet being built */
>  	u64 rsc_count;			/* stat for coalesced packets */
>  	u64 rsc_flush;			/* stats for flushed packets */
>  	u32 restart_queue;		/* track tx queue restarts */
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index c35e13c..43987a4 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -1130,36 +1130,6 @@ static inline u32 ixgbe_get_rsc_count(union
> ixgbe_adv_rx_desc *rx_desc)
>  		IXGBE_RXDADV_RSCCNT_SHIFT;
>  }
> 
> -/**
> - * ixgbe_transform_rsc_queue - change rsc queue into a full packet
> - * @skb: pointer to the last skb in the rsc queue
> - * @count: pointer to number of packets coalesced in this context
> - *
> - * This function changes a queue full of hw rsc buffers into a completed
> - * packet.  It uses the ->prev pointers to find the first packet and then
> - * turns it into the frag list owner.
> - **/
> -static inline struct sk_buff *ixgbe_transform_rsc_queue(struct sk_buff *skb,
> -							u64 *count)
> -{
> -	unsigned int frag_list_size = 0;
> -
> -	while (skb->prev) {
> -		struct sk_buff *prev = skb->prev;
> -		frag_list_size += skb->len;
> -		skb->prev = NULL;
> -		skb = prev;
> -		*count += 1;
> -	}
> -
> -	skb_shinfo(skb)->frag_list = skb->next;
> -	skb->next = NULL;
> -	skb->len += frag_list_size;
> -	skb->data_len += frag_list_size;
> -	skb->truesize += frag_list_size;
> -	return skb;
> -}
> -
>  struct ixgbe_rsc_cb {
>  	dma_addr_t dma;
>  	bool delay_unmap;
> @@ -1216,10 +1186,13 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector
> *q_vector,
>  		prefetch(skb->data);
>  		rx_buffer_info->skb = NULL;
> 
> +		if (!(staterr & IXGBE_RXD_STAT_EOP)) {
> +			rx_ring->rsc_skb = skb;
> +			rx_ring->rsc_count++;
> +		}
>  		if (rx_buffer_info->dma) {
>  			if ((adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
> -			    (!(staterr & IXGBE_RXD_STAT_EOP)) &&
> -				 (!(skb->prev))) {
> +			    rx_ring->rsc_skb == skb) {
>  				/*
>  				 * When HWRSC is enabled, delay unmapping
>  				 * of the first packet. It carries the
> @@ -1279,9 +1252,12 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector
> *q_vector,
>  		}
> 
>  		if (staterr & IXGBE_RXD_STAT_EOP) {
> -			if (skb->prev)
> -				skb = ixgbe_transform_rsc_queue(skb,
> -
> 	&(rx_ring->rsc_count));
> +			if (rx_ring->rsc_skb) {
> +				skb = rx_ring->rsc_skb;
> +				rx_ring->rsc_skb = NULL;
> +
> +				skb->prev = NULL;
> +			}
>  			if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
>  				if (IXGBE_RSC_CB(skb)->delay_unmap) {
>  					dma_unmap_single(&pdev->dev,
> @@ -1307,8 +1283,22 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector
> *q_vector,
>  				next_buffer->skb = skb;
>  				next_buffer->dma = 0;
>  			} else {
> -				skb->next = next_buffer->skb;
> -				skb->next->prev = skb;
> +				struct sk_buff *head = rx_ring->rsc_skb;
> +				struct sk_buff *next = next_buffer->skb;
> +
> +				if (!skb_shinfo(head)->frag_list) {
> +					skb_shinfo(head)->frag_list =
> next;
> +				} else {
> +					head->prev->next = next;
> +				}
> +
> +				/* ->prev tracks the last skb in the
> frag_list */
> +				head->prev = next;
> +				rx_ring->rsc_count++;
> +
> +				head->len += next->len;
> +				head->data_len += next->len;
> +				head->truesize += next->len;
>  			}
>  			rx_ring->non_eop_descs++;
>  			goto next_desc;
> --
> 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: ixgbe: normalize frag_list usage

[Alexander Duyck]

David Miller wrote:
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
> 
> Basically I want to get the whole tree to consistently use
> the convention that the frag list is built as packets arrive
> by using "skb->prev" of the head skb to track the tail member
> of the frag list.
> 
> This will allow me to do something like:
> 
> struct sk_buff {
>        union {
> 		struct {
> 			struct sk_buff *next;
> 			struct sk_buff *prev;
> 		};
> 		struct {
> 			struct sk_buff *frag_next;
> 			struct sk_buff *frag_tail_tracker;
> 		};
> 	}
>  ...
> }
> 
> And have all frag_list code use these members consistently.
> 
> While doing this patch I noticed that there are some left-over bits in
> the IXGBEVF driver that builds these IXGBE style (before my patch)
> skb->prev RSC chains.
> 
> But nothing other than the RX ring shutdown code processes them, so
> likely if they are created they will leak or cause some other kind of
> problem.
> 
> Please take a look, thanks.
> 
> diff --git a/drivers/net/ixgbe/ixgbe.h b/drivers/net/ixgbe/ixgbe.h
> index 5cebc37..434c9fa 100644
> --- a/drivers/net/ixgbe/ixgbe.h
> +++ b/drivers/net/ixgbe/ixgbe.h
> @@ -181,6 +181,7 @@ struct ixgbe_ring {
>  	struct ixgbe_queue_stats stats;
>  	unsigned long reinit_state;
>  	int numa_node;
> +	struct sk_buff *rsc_skb;	/* RSC packet being built */
>  	u64 rsc_count;			/* stat for coalesced packets */
>  	u64 rsc_flush;			/* stats for flushed packets */
>  	u32 restart_queue;		/* track tx queue restarts */

This will not work for RSC due to the fact that it assumes only one RSC 
context is active per ring and that is not the case.  There can be 
multiple RSC combined flows interlaced on the ring.

It occurs to me that RSC needs the inverse of what you need for frag 
list handling.  You state that you need the next and tail pointers, 
while RSC really needs the prev and head pointers.  One possible 
solution for all this would be to just reverse the logic a bit.

What you are setting up right now is essentially a head based layout 
based on the idea that th head it the component you will have access to. 
   RSC on the other hand is a tail based layout.  If we were to overload 
skb->next so that it pointed at head from tail in ixgbe then we could 
make RSC function, and probably even improve the performance a bit by 
getting rid of the extra RSC transform at the end of creating RSC packets.

Below I have made suggestions based on a tail based approach if we 
overloaded the next pointer on the tail packet to point to the head.

> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index c35e13c..43987a4 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -1130,36 +1130,6 @@ static inline u32 ixgbe_get_rsc_count(union ixgbe_adv_rx_desc *rx_desc)
>  		IXGBE_RXDADV_RSCCNT_SHIFT;
>  }
>  
> -/**
> - * ixgbe_transform_rsc_queue - change rsc queue into a full packet
> - * @skb: pointer to the last skb in the rsc queue
> - * @count: pointer to number of packets coalesced in this context
> - *
> - * This function changes a queue full of hw rsc buffers into a completed
> - * packet.  It uses the ->prev pointers to find the first packet and then
> - * turns it into the frag list owner.
> - **/
> -static inline struct sk_buff *ixgbe_transform_rsc_queue(struct sk_buff *skb,
> -							u64 *count)
> -{
> -	unsigned int frag_list_size = 0;
> -
> -	while (skb->prev) {
> -		struct sk_buff *prev = skb->prev;
> -		frag_list_size += skb->len;
> -		skb->prev = NULL;
> -		skb = prev;
> -		*count += 1;
> -	}
> -
> -	skb_shinfo(skb)->frag_list = skb->next;
> -	skb->next = NULL;
> -	skb->len += frag_list_size;
> -	skb->data_len += frag_list_size;
> -	skb->truesize += frag_list_size;
> -	return skb;
> -}
> -
>  struct ixgbe_rsc_cb {
>  	dma_addr_t dma;
>  	bool delay_unmap;

So this could probably go since we wouldn't need it for the new approach.

> @@ -1216,10 +1186,13 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
>  		prefetch(skb->data);
>  		rx_buffer_info->skb = NULL;
>  
> +		if (!(staterr & IXGBE_RXD_STAT_EOP)) {
> +			rx_ring->rsc_skb = skb;
> +			rx_ring->rsc_count++;
> +		}
>  		if (rx_buffer_info->dma) {
>  			if ((adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
> -			    (!(staterr & IXGBE_RXD_STAT_EOP)) &&
> -				 (!(skb->prev))) {
> +			    rx_ring->rsc_skb == skb) {
>  				/*
>  				 * When HWRSC is enabled, delay unmapping
>  				 * of the first packet. It carries the

This bit of code is based on the flawed assumption that RSC doesn't have 
other packets interlaced with is not true.

> @@ -1279,9 +1252,12 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
>  		}
>  
>  		if (staterr & IXGBE_RXD_STAT_EOP) {
> -			if (skb->prev)
> -				skb = ixgbe_transform_rsc_queue(skb,
> -								&(rx_ring->rsc_count));
> +			if (rx_ring->rsc_skb) {
> +				skb = rx_ring->rsc_skb;
> +				rx_ring->rsc_skb = NULL;
> +
> +				skb->prev = NULL;
> +			}
>  			if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
>  				if (IXGBE_RSC_CB(skb)->delay_unmap) {
>  					dma_unmap_single(&pdev->dev,

So this bit of code could be changed to something like:
			if (skb->next) {
				skb = skb->next;
				skb->frag_tail_tracker->next == NULL;
			}

> @@ -1307,8 +1283,22 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
>  				next_buffer->skb = skb;
>  				next_buffer->dma = 0;
>  			} else {
> -				skb->next = next_buffer->skb;
> -				skb->next->prev = skb;
> +				struct sk_buff *head = rx_ring->rsc_skb;
> +				struct sk_buff *next = next_buffer->skb;
> +
> +				if (!skb_shinfo(head)->frag_list) {
> +					skb_shinfo(head)->frag_list = next;
> +				} else {
> +					head->prev->next = next;
> +				}
> +
> +				/* ->prev tracks the last skb in the frag_list */
> +				head->prev = next;
> +				rx_ring->rsc_count++;
> +		
> +				head->len += next->len;
> +				head->data_len += next->len;
> +				head->truesize += next->len;
>  			}
>  			rx_ring->non_eop_descs++;
>  			goto next_desc;

This piece is a bit trickier, the len/data_len/truesize addition needs 
to be done on all packets and it looks like it was only done on the 
non-EOP packets which may be an issue.  My guess would be that it would 
probably work if we added something along of the lines of the following 
before the EOP check:
		if (skb->next) {
			skb->next->len += skb->len;
			skb->next->data_len += skb->data_len;
			skb->next->truesize += skb->truesize;
			rx_ring->rsc_count++;
		}

My thoughts for the rest of the code for the non-EOP case would be 
something more along the lines of:
		if (skb->next) {
			next_buffer->skb->next = skb->next;
			skb->next->frag_tail_tracker = next_buffer->skb;
			skb->next = next_buffer->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: ixgbe: normalize frag_list usage

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Mon, 04 Oct 2010 11:04:18 -0700

> This will not work for RSC due to the fact that it assumes only one
> RSC context is active per ring and that is not the case.  There can be
> multiple RSC combined flows interlaced on the ring.

Thanks for looking at this Alexander.

I must have mis-understood what the current code is doing.

It looked like RSC packets always show up in-order in the RX ring.

And that they are scanned for by simply combining any sequence of
non-EOP packets up to and including the final EOP one into a frag
list.

Are the RSC packets are identified via something else entirely?

Re: ixgbe: normalize frag_list usage

[Alexander Duyck]

David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> Date: Mon, 04 Oct 2010 11:04:18 -0700
> 
>> This will not work for RSC due to the fact that it assumes only one
>> RSC context is active per ring and that is not the case.  There can be
>> multiple RSC combined flows interlaced on the ring.
> 
> Thanks for looking at this Alexander.
> 
> I must have mis-understood what the current code is doing.
> 
> It looked like RSC packets always show up in-order in the RX ring.
> 
> And that they are scanned for by simply combining any sequence of
> non-EOP packets up to and including the final EOP one into a frag
> list.
> 
> Are the RSC packets are identified via something else entirely?

They show up in order, but they are not necessarily linear.  You can 
have other packets show up in the middle of the flow and RSC just jumps 
over them.  The determination for the jump is handled via nextp in 
ixgbe_clean_rx_irq.

I'll look into this and see if I can come up with a counter-proposal 
patch based on the suggestions I was making.  The key item though is 
that the tail would need some means of referencing head which I think 
can probably be accomplished via skb->next.

Thanks,

Alex

RE: ixgbe: normalize frag_list usage

[Duyck, Alexander H]

>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net]
>Sent: Monday, October 04, 2010 11:33 AM
>To: Duyck, Alexander H
>Cc: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W;
>netdev@vger.kernel.org
>Subject: Re: ixgbe: normalize frag_list usage
>
>From: Alexander Duyck <alexander.h.duyck@intel.com>
>Date: Mon, 04 Oct 2010 11:04:18 -0700
>
>> This will not work for RSC due to the fact that it assumes only one
>> RSC context is active per ring and that is not the case.  There can
>be
>> multiple RSC combined flows interlaced on the ring.
>
>Thanks for looking at this Alexander.
>
>I must have mis-understood what the current code is doing.
>
>It looked like RSC packets always show up in-order in the RX ring.
>
>And that they are scanned for by simply combining any sequence of
>non-EOP packets up to and including the final EOP one into a frag
>list.
>
>Are the RSC packets are identified via something else entirely?

Dave,

The patch below is kind of what I had in mind for a way to do RSC and maintain
the pointer scheme you are looking for.  Consider this patch an RFC for now
since I based this off of Jeff's internal testing tree and so it would need
some modification to apply cleanly to net-next.

The only deviation from a standard frag list is that we are appending the tail
before it actually has any data in it so we have to break things into three
steps.  One to add the tail to the end of the frag list, one to merge the
length from the tail into the head, and finally one to clean-up the head->prev
pointer.

If this works for you I will send it to Jeff to add to his tree for testing.

Thanks,

Alex

---

ixgbe: change RSC to use a normalize frag_list usage

From: Alexander Duyck <alexander.h.duyck@intel.com>

This change drops the RSC queue approach and instead creates a normalized
frag_list skb but the tail is kept active and regularly merged into the
host SKB every time it is completed.  In order to identify the tail skb
as a tail we set the next pointer to the head skb, and the head skb prev
pointer to the tail.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 drivers/net/ixgbe/ixgbe.h      |    2 -
 drivers/net/ixgbe/ixgbe_main.c |   96 +++++++++++++++++++++++-----------------
 2 files changed, 56 insertions(+), 42 deletions(-)


diff --git a/drivers/net/ixgbe/ixgbe.h b/drivers/net/ixgbe/ixgbe.h
index b9a1e70..2c7a296 100644
--- a/drivers/net/ixgbe/ixgbe.h
+++ b/drivers/net/ixgbe/ixgbe.h
@@ -470,7 +470,7 @@ enum ixbge_state_t {
 
 struct ixgbe_rsc_cb {
 	dma_addr_t dma;
-	u16 skb_cnt;
+	u16 append_cnt;
 	bool delay_unmap;
 };
 #define IXGBE_RSC_CB(skb) ((struct ixgbe_rsc_cb *)(skb)->cb)
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 221d5ef..c3774b9 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -1251,34 +1251,46 @@ static inline u16 ixgbe_get_hlen(union ixgbe_adv_rx_desc *rx_desc)
 }
 
 /**
- * ixgbe_transform_rsc_queue - change rsc queue into a full packet
- * @skb: pointer to the last skb in the rsc queue
+ * ixgbe_merge_active_tail - merge active tail into frag_list skb
+ * @tail: pointer to active tail in frag_list
  *
- * This function changes a queue full of hw rsc buffers into a completed
- * packet.  It uses the ->prev pointers to find the first packet and then
- * turns it into the frag list owner.
+ * This function merges the length and data of an active tail into the
+ * skb containing the frag_list.  It resets the tail's pointer to the head,
+ * but it leaves the heads pointer to tail intact.
  **/
-static inline struct sk_buff *ixgbe_transform_rsc_queue(struct sk_buff *skb)
+static inline struct sk_buff *ixgbe_merge_active_tail(struct sk_buff *tail)
 {
-	unsigned int frag_list_size = 0;
-	unsigned int skb_cnt = 1;
-
-	while (skb->prev) {
-		struct sk_buff *prev = skb->prev;
-		frag_list_size += skb->len;
-		skb->prev = NULL;
-		skb = prev;
-		skb_cnt++;
+	if (tail->next) {
+		struct sk_buff *head = tail->next;
+		head->len += tail->len;
+		head->data_len += tail->len;
+		head->truesize += tail->len;
+		tail->next = NULL;
+		return head;
 	}
+	return tail;
+}
 
-	skb_shinfo(skb)->frag_list = skb->next;
-	skb->next = NULL;
-	skb->len += frag_list_size;
-	skb->data_len += frag_list_size;
-	skb->truesize += frag_list_size;
-	IXGBE_RSC_CB(skb)->skb_cnt = skb_cnt;
-
-	return skb;
+/**
+ * ixgbe_append_active_tail - add empty skb to the end of a frag_list
+ * @head: skb that will host the frag_list
+ * @tail: skb to add to the end of the frag_list
+ *
+ * This function adds a yet to be completed skb to the end of a frag_list.
+ * It is an in-between step while we are still waiting for the data to be
+ * placed into the yet to be completed skb.  A back pointer from tail to
+ * head is placed in tail->next so we can later merge it into the host skb.
+ * The head->prev pointer is used to track the tail of the frag_list.
+ **/
+static inline void ixgbe_append_active_tail(struct sk_buff *head,
+					    struct sk_buff *tail)
+{
+	if (skb_shinfo(head)->frag_list)
+		head->prev->next = tail;
+	else
+		skb_shinfo(head)->frag_list = tail;
+	head->prev = tail;
+	tail->next = head;
 }
 
 static inline bool ixgbe_get_rsc_state(union ixgbe_adv_rx_desc *rx_desc)
@@ -1328,7 +1340,7 @@ static void ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 			u16 hlen;
 			if (pkt_is_rsc &&
 			    !(staterr & IXGBE_RXD_STAT_EOP) &&
-			    !skb->prev) {
+			    !skb->next) {
 				/*
 				 * When HWRSC is enabled, delay unmapping
 				 * of the first packet. It carries the
@@ -1397,6 +1409,8 @@ static void ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 			next_buffer = &rx_ring->rx_buffer_info[i];
 		}
 
+		skb = ixgbe_merge_active_tail(skb);
+
 		if (!(staterr & IXGBE_RXD_STAT_EOP)) {
 			if (ring_is_ps_enabled(rx_ring)) {
 				rx_buffer_info->skb = next_buffer->skb;
@@ -1404,15 +1418,16 @@ static void ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 				next_buffer->skb = skb;
 				next_buffer->dma = 0;
 			} else {
-				skb->next = next_buffer->skb;
-				skb->next->prev = skb;
+				ixgbe_append_active_tail(skb,
+							 next_buffer->skb);
+				IXGBE_RSC_CB(skb)->append_cnt++;
 			}
 			rx_ring->rx_stats.non_eop_descs++;
 			goto next_desc;
 		}
 
 		if (skb->prev) {
-			skb = ixgbe_transform_rsc_queue(skb);
+			skb->prev = NULL;
 			/* if we got here without RSC the packet is invalid */
 			if (!pkt_is_rsc) {
 				__pskb_trim(skb, 0);
@@ -1437,7 +1452,7 @@ static void ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 					skb_shinfo(skb)->nr_frags;
 			else
 				rx_ring->rx_stats.rsc_count +=
-					IXGBE_RSC_CB(skb)->skb_cnt;
+					IXGBE_RSC_CB(skb)->append_cnt + 1;
 			rx_ring->rx_stats.rsc_flush++;
 		}
 
@@ -3907,19 +3922,18 @@ static void ixgbe_clean_rx_ring(struct ixgbe_ring *rx_ring)
 		if (rx_buffer_info->skb) {
 			struct sk_buff *skb = rx_buffer_info->skb;
 			rx_buffer_info->skb = NULL;
-			do {
-				struct sk_buff *this = skb;
-				if (IXGBE_RSC_CB(this)->delay_unmap) {
-					dma_unmap_single(dev,
-							 IXGBE_RSC_CB(this)->dma,
-							 rx_ring->rx_buf_len,
-							 DMA_FROM_DEVICE);
-					IXGBE_RSC_CB(this)->dma = 0;
-					IXGBE_RSC_CB(skb)->delay_unmap = false;
-				}
-				skb = skb->prev;
-				dev_kfree_skb(this);
-			} while (skb);
+			/* We need to clean up RSC frag lists */
+			skb = ixgbe_merge_active_tail(skb);
+			skb->prev = NULL;
+			if (IXGBE_RSC_CB(skb)->delay_unmap) {
+				dma_unmap_single(dev,
+						 IXGBE_RSC_CB(skb)->dma,
+						 rx_ring->rx_buf_len,
+						 DMA_FROM_DEVICE);
+				IXGBE_RSC_CB(skb)->dma = 0;
+				IXGBE_RSC_CB(skb)->delay_unmap = false;
+			}
+			dev_kfree_skb(skb);
 		}
 		if (!rx_buffer_info->page)
 			continue;

Re: ixgbe: normalize frag_list usage

[David Miller]

From: "Duyck, Alexander H" <alexander.h.duyck@intel.com>
Date: Tue, 5 Oct 2010 15:45:32 -0700

> The patch below is kind of what I had in mind for a way to do RSC
> and maintain the pointer scheme you are looking for.  Consider this
> patch an RFC for now since I based this off of Jeff's internal
> testing tree and so it would need some modification to apply cleanly
> to net-next.

Thanks a lot for doing this work Alex.

I'll take a look and give you some feedback soon.

Re: ixgbe: normalize frag_list usage

[David Miller]

From: "Duyck, Alexander H" <alexander.h.duyck@intel.com>
Date: Tue, 5 Oct 2010 15:45:32 -0700

> The patch below is kind of what I had in mind for a way to do RSC and maintain
> the pointer scheme you are looking for.  Consider this patch an RFC for now
> since I based this off of Jeff's internal testing tree and so it would need
> some modification to apply cleanly to net-next.

Can you really not remember the head somewhere?

What I wanted is for everyone to build their frag list SKBs from head to tail,
always.  So that I could, as I mentioned in my original posting, do something
like:

struct sk_buff {
	union {
		struct list_head list;
		struct {
			struct sk_buff	*frag_next;
			struct sk_buff	*frag_tail_tracker;
		};
	};
};

The ->frag_tail_tracker is only used in the head SKB to maintain where the
last SKB in the frag list is.

You're tracking the head from the inner SKBs, such that my intended
conventions are not being followed.

Re: ixgbe: normalize frag_list usage

[Alexander Duyck]

On 10/6/2010 11:58 PM, David Miller wrote:
> From: "Duyck, Alexander H"<alexander.h.duyck@intel.com>
> Date: Tue, 5 Oct 2010 15:45:32 -0700
>
>> The patch below is kind of what I had in mind for a way to do RSC and maintain
>> the pointer scheme you are looking for.  Consider this patch an RFC for now
>> since I based this off of Jeff's internal testing tree and so it would need
>> some modification to apply cleanly to net-next.
>
> Can you really not remember the head somewhere?

I can track it in the RSC_CB if that works for you.  Right now though I 
guess I am not seeing the difference between tracking this in 
skb->frag_next vs IXGBE_RSC_CB(skb)->frag_head.  I think it might help 
if you were to provide some functions that demonstrate exactly what you 
had in mind for frag list handling.  Specifically if you were to add a 
function for merging a frag into the frag list, and for how you want to 
approach cleaning up the skb->prev/frag_tail_tracker pointer when you 
are cleaning up an active frag_list.

> What I wanted is for everyone to build their frag list SKBs from head to tail,
> always.  So that I could, as I mentioned in my original posting, do something
> like:

My change is doing just that.  It is building from head to tail.  The 
only difference is that tail is tracking head since it has to be updated 
after the tail is added, and then I can drop next/frag_next back to NULL.

> struct sk_buff {
> 	union {
> 		struct list_head list;
> 		struct {
> 			struct sk_buff	*frag_next;
> 			struct sk_buff	*frag_tail_tracker;
> 		};
> 	};
> };
>
> The ->frag_tail_tracker is only used in the head SKB to maintain where the
> last SKB in the frag list is.

That is what I was doing in the head skb.

> You're tracking the head from the inner SKBs, such that my intended
> conventions are not being followed.

What I am doing is tracking the head from the tail skb.  The reason for 
this is that I need some way to update the len, data_len, and truesize 
after I have placed data in the tail via skb_put.  All of the other SKBs 
in the frag list are just tracking the next like any other SKB in the 
frag_list.

Now that I think about it I guess the issue is that I am adding the SKB 
to the frag_list before it is complete.  I will send another patch out 
in the next couple of hours that should address most of the issues.

Thanks,

Alex

[RFC] ixgbe: v3 normalize frag_list usage

[Duyck, Alexander H]

Dave,

Below is the new and improved version of the RSC chaining approach.  Basically
I am holding off on merging the SKB into the frame until the SKB has data in
order to make it take a more standard approach.

Let me know if this will work with the new pointer structure.

Thanks,

Alex

---

This change drops the RSC queue approach and instead creates a normalized
frag_list skb but the tail is kept active and regularly merged into the
host SKB every time it is completed.  In order to identify the tail skb
as a tail we set the head pointer in the RSC CB block of the skb. To locate
the head we just need to check to see if skb->prev is set and make sure to
clean up the pointer before we pass it up to the stack.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 drivers/net/ixgbe/ixgbe.h      |    3 +
 drivers/net/ixgbe/ixgbe_main.c |   96 +++++++++++++++++++++++-----------------
 2 files changed, 58 insertions(+), 41 deletions(-)


diff --git a/drivers/net/ixgbe/ixgbe.h b/drivers/net/ixgbe/ixgbe.h
index a62d19c..397f9e1 100644
--- a/drivers/net/ixgbe/ixgbe.h
+++ b/drivers/net/ixgbe/ixgbe.h
@@ -469,8 +469,9 @@ enum ixbge_state_t {
 };
 
 struct ixgbe_rsc_cb {
+	struct sk_buff *head;
 	dma_addr_t dma;
-	u16 skb_cnt;
+	u16 append_cnt;
 	bool delay_unmap;
 };
 #define IXGBE_RSC_CB(skb) ((struct ixgbe_rsc_cb *)(skb)->cb)
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 8edce66..104a833 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -1251,34 +1251,51 @@ static inline u16 ixgbe_get_hlen(union ixgbe_adv_rx_desc *rx_desc)
 }
 
 /**
- * ixgbe_transform_rsc_queue - change rsc queue into a full packet
- * @skb: pointer to the last skb in the rsc queue
+ * ixgbe_merge_active_tail - merge active tail into frag_list skb
+ * @tail: pointer to active tail in frag_list
  *
- * This function changes a queue full of hw rsc buffers into a completed
- * packet.  It uses the ->prev pointers to find the first packet and then
- * turns it into the frag list owner.
+ * This function merges the length and data of an active tail into the
+ * skb containing the frag_list.  It resets the tail's pointer to the head,
+ * but it leaves the heads pointer to tail intact.
  **/
-static inline struct sk_buff *ixgbe_transform_rsc_queue(struct sk_buff *skb)
+static inline struct sk_buff *ixgbe_merge_active_tail(struct sk_buff *tail)
 {
-	unsigned int frag_list_size = 0;
-	unsigned int skb_cnt = 1;
+	struct sk_buff *head = IXGBE_RSC_CB(tail)->head;
 
-	while (skb->prev) {
-		struct sk_buff *prev = skb->prev;
-		frag_list_size += skb->len;
-		skb->prev = NULL;
-		skb = prev;
-		skb_cnt++;
-	}
+	if (!head)
+		return tail;
+
+	IXGBE_RSC_CB(tail)->head = NULL;
+
+	if (head->prev)
+		head->prev->next = tail;
+	else
+		skb_shinfo(head)->frag_list = tail;
+
+	head->len += tail->len;
+	head->data_len += tail->len;
+	head->truesize += tail->len;
 
-	skb_shinfo(skb)->frag_list = skb->next;
-	skb->next = NULL;
-	skb->len += frag_list_size;
-	skb->data_len += frag_list_size;
-	skb->truesize += frag_list_size;
-	IXGBE_RSC_CB(skb)->skb_cnt = skb_cnt;
+	head->prev = tail;
+	IXGBE_RSC_CB(head)->append_cnt++;
 
-	return skb;
+	return head;
+}
+
+/**
+ * ixgbe_close_active_frag_list - cleanup pointers on a frag_list skb
+ * @head: pointer to head of an active frag list
+ *
+ * This function will clear the frag_tail_tracker pointer on an active
+ * frag_list and returns true if the pointer was actually set
+ **/
+static inline bool ixgbe_close_active_frag_list(struct sk_buff *head)
+{
+	if (head->prev) {
+		head->prev = NULL;
+		return true;
+	}
+	return false;
 }
 
 static inline bool ixgbe_get_rsc_state(union ixgbe_adv_rx_desc *rx_desc)
@@ -1397,6 +1414,8 @@ static void ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 			next_buffer = &rx_ring->rx_buffer_info[i];
 		}
 
+		skb = ixgbe_merge_active_tail(skb);
+
 		if (!(staterr & IXGBE_RXD_STAT_EOP)) {
 			if (ring_is_ps_enabled(rx_ring)) {
 				rx_buffer_info->skb = next_buffer->skb;
@@ -1404,15 +1423,13 @@ static void ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 				next_buffer->skb = skb;
 				next_buffer->dma = 0;
 			} else {
-				skb->next = next_buffer->skb;
-				skb->next->prev = skb;
+				IXGBE_RSC_CB(next_buffer->skb)->head = skb;
 			}
 			rx_ring->rx_stats.non_eop_descs++;
 			goto next_desc;
 		}
 
-		if (skb->prev) {
-			skb = ixgbe_transform_rsc_queue(skb);
+		if (ixgbe_close_active_frag_list(skb)) {
 			/* if we got here without RSC the packet is invalid */
 			if (!pkt_is_rsc) {
 				__pskb_trim(skb, 0);
@@ -1437,7 +1454,7 @@ static void ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 					skb_shinfo(skb)->nr_frags;
 			else
 				rx_ring->rx_stats.rsc_count +=
-					IXGBE_RSC_CB(skb)->skb_cnt;
+					IXGBE_RSC_CB(skb)->append_cnt;
 			rx_ring->rx_stats.rsc_flush++;
 		}
 
@@ -3907,19 +3924,18 @@ static void ixgbe_clean_rx_ring(struct ixgbe_ring *rx_ring)
 		if (rx_buffer_info->skb) {
 			struct sk_buff *skb = rx_buffer_info->skb;
 			rx_buffer_info->skb = NULL;
-			do {
-				struct sk_buff *this = skb;
-				if (IXGBE_RSC_CB(this)->delay_unmap) {
-					dma_unmap_single(dev,
-							 IXGBE_RSC_CB(this)->dma,
-							 rx_ring->rx_buf_len,
-							 DMA_FROM_DEVICE);
-					IXGBE_RSC_CB(this)->dma = 0;
-					IXGBE_RSC_CB(skb)->delay_unmap = false;
-				}
-				skb = skb->prev;
-				dev_kfree_skb(this);
-			} while (skb);
+			/* We need to clean up RSC frag lists */
+			skb = ixgbe_merge_active_tail(skb);
+			ixgbe_close_active_frag_list(skb);
+			if (IXGBE_RSC_CB(skb)->delay_unmap) {
+				dma_unmap_single(dev,
+						 IXGBE_RSC_CB(skb)->dma,
+						 rx_ring->rx_buf_len,
+						 DMA_FROM_DEVICE);
+				IXGBE_RSC_CB(skb)->dma = 0;
+				IXGBE_RSC_CB(skb)->delay_unmap = false;
+			}
+			dev_kfree_skb(skb);
 		}
 		if (!rx_buffer_info->page)
 			continue;

Re: ixgbe: normalize frag_list usage

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Thu, 07 Oct 2010 12:59:14 -0700

> I can track it in the RSC_CB if that works for you.  Right now though
> I guess I am not seeing the difference between tracking this in
> skb->frag_next vs IXGBE_RSC_CB(skb)->frag_head.  I think it might help
> if you were to provide some functions that demonstrate exactly what
> you had in mind for frag list handling.  Specifically if you were to
> add a function for merging a frag into the frag list, and for how you
> want to approach cleaning up the skb->prev/frag_tail_tracker pointer
> when you are cleaning up an active frag_list.

Basically the one helper function will look like this.

static inline void skb_frag_list_add(struct sk_buff *head,
				     struct sk_buff *new)
{
	if (!skb_shinfo(skb)->frag_list) {
		skb_shinfo(skb)->frag_list = new;
		skb->frag_list_tail = new;
	} else {
		skb->frag_list_tail->frag_list_next = new;
		skb->frag_list_tail = new;
	}
}

If you have to track the head from the tail packets, please do
so in a private control block.

Re: [RFC] ixgbe: v3 normalize frag_list usage

[David Miller]

From: "Duyck, Alexander H" <alexander.h.duyck@intel.com>
Date: Fri, 8 Oct 2010 16:57:40 -0700

> Below is the new and improved version of the RSC chaining approach.  Basically
> I am holding off on merging the SKB into the frame until the SKB has data in
> order to make it take a more standard approach.
> 
> Let me know if this will work with the new pointer structure.

This variant looks great, thanks Alexander!