Skip to contentSkip to conversation

Question on e1000 patch, rx-copy-break related.

DISCUSSION Ben Greear 3 participants 6 messages May 2, 2006 – May 4, 2006

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Ben Greear

author
#0 lore raw

In commit: a292ca6efbc1f259ddfb9c902367f2588e0e8b0f to e1000_main.c, there is the change below.

I am curious why the skb_put no longer subtracts ETHERNET_FCS_SIZE from the length. Is the idea that we will now always include the FCS at the end of the skb?

It also seems that the skb_put for the else clause is missing here, but I think it is fixed in some later patch.

The main reason I ask is that I have a patch that enabled reception of the FCS in 2.6.13. It used a flag to determine whether to subtract the ETHERNET_FCS_SIZE from length (or not, if we are capturing FCS). I am trying to figure out if this is still needed in the later releases.

Thanks,
Ben

@@ -3613,17 +3618,40 @@ #endif
}
}

- /* Good Receive */
- skb_put(skb, length - ETHERNET_FCS_SIZE);
+ /* code added for copybreak, this should improve
+ * performance for small packets with large amounts
+ * of reassembly being done in the stack */
+#define E1000_CB_LENGTH 256
+ if ((length < E1000_CB_LENGTH) &&

+		   !rx_ring->rx_skb_top &&
+		   /* or maybe (status & E1000_RXD_STAT_EOP) && */
+		   !multi_descriptor) {

+ struct sk_buff *new_skb =
+ dev_alloc_skb(length + NET_IP_ALIGN);
+ if (new_skb) {
+ skb_reserve(new_skb, NET_IP_ALIGN);
+ new_skb->dev = netdev;
+ memcpy(new_skb->data - NET_IP_ALIGN,
+ skb->data - NET_IP_ALIGN,
+ length + NET_IP_ALIGN);
+ /* save the skb in buffer_info as good */
+ buffer_info->skb = skb;
+ skb = new_skb;
+ skb_put(skb, length);
+ }
+ }
+
+ /* end copybreak code */

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#1 lore raw
Quoted 6 lines · Ben Greear

In commit: a292ca6efbc1f259ddfb9c902367f2588e0e8b0f to e1000_main.c, there is the change below.

I am curious why the skb_put no longer subtracts ETHERNET_FCS_SIZE from the length. Is the idea that we will now always include the FCS at the end of the skb?

This is a long and hairy story behind this, but there is a bit called SECRC that controls hardware stripping of the CRC. In *this* driver we turned that bit on, so that we didn't have to mess with skb->len -= 4 and the nasty negative unwrap if we were using multiple descriptors for rx.

Since then, we've removed multiple descriptor rx.

And after that, I've discovered that some BMCs are very unhappy if we strip CRCs using SECRC.

So, my related question is: is it okay if we just always leave the CRC on the end of the data? It doesn't seem to harm anything.

Quoted 7 lines

It also seems that the skb_put for the else clause is missing here, but I think it is fixed in some later patch.

The main reason I ask is that I have a patch that enabled reception of the FCS in 2.6.13. It used a flag to determine whether to subtract the ETHERNET_FCS_SIZE from length (or not, if we are capturing FCS). I am trying to figure out if this is still needed in the later releases.

Well, its a changing picture. I had planned to eventually enable the hardware to strip the CRC if we aren't connected to some kind of offboard management. We'll get there in steps.

Quoted 33 lines

Thanks,
Ben

(quoted diff) +24 -2
@@ -3613,17 +3618,40 @@ #endif
                        }
                }
 
-               /* Good Receive */
-               skb_put(skb, length - ETHERNET_FCS_SIZE);
+               /* code added for copybreak, this should improve
+                * performance for small packets with large amounts
+                * of reassembly being done in the stack */
+#define E1000_CB_LENGTH 256
+               if ((length < E1000_CB_LENGTH) &&
+                  !rx_ring->rx_skb_top &&
+                  /* or maybe (status & E1000_RXD_STAT_EOP) && */
+                  !multi_descriptor) {
+                       struct sk_buff *new_skb =
+                           dev_alloc_skb(length + NET_IP_ALIGN);
+                       if (new_skb) {
+                               skb_reserve(new_skb, NET_IP_ALIGN);
+                               new_skb->dev = netdev;
+                               memcpy(new_skb->data - NET_IP_ALIGN,
+                                      skb->data - NET_IP_ALIGN,
+                                      length + NET_IP_ALIGN);
+                               /* save the skb in buffer_info as good */
+                               buffer_info->skb = skb;
+                               skb = new_skb;
+                               skb_put(skb, length);
+                       }
+               }
+
+               /* end copybreak code */
author
#2 lore raw
Quoted 16 lines · Jesse Brandeburg
Quoted 6 lines · Ben Greear

In commit: a292ca6efbc1f259ddfb9c902367f2588e0e8b0f to e1000_main.c, there is the change below.

I am curious why the skb_put no longer subtracts ETHERNET_FCS_SIZE from the length. Is the idea that we will now always include the FCS at the end of the skb?

This is a long and hairy story behind this, but there is a bit called SECRC that controls hardware stripping of the CRC. In *this* driver we turned that bit on, so that we didn't have to mess with skb->len -= 4 and the nasty negative unwrap if we were using multiple descriptors for rx.

Quoted 7 lines

Since then, we've removed multiple descriptor rx.

And after that, I've discovered that some BMCs are very unhappy if we strip CRCs using SECRC.

So, my related question is: is it okay if we just always leave the CRC on the end of the data? It doesn't seem to harm anything.

I believe it might mess up bridging logic if it tries to send the entire skb to a peer interface, ie cause an extra 4 bytes to be written.

I know that this 'save-crc' feature did mess up my particular bridge-like thing, but that could have been just bugs in my code.

Also, if the CRC data is there, but it is never 'put' into the skb, then it will be correctly ignored by the stacks, including bridging, and hacks such as my own that simply increase the skb-put by 4 bytes will work.

My personal preference is to set a flag in the skb struct indicating whether or not the crc is appended (and skb_put). Then, bridging code can ignore it if needed, and sniffers and such can get the CRC in user-land. To remain backwards compat, at least the skb-put of the crc logic should default to OFF so that we don't break any existing user-land bridging logic. I have the ethtool API logic written to twiddle this save-crc behaviour if someone decides this is worthy of the kernel.

Quoted 3 lines

Well, its a changing picture. I had planned to eventually enable the hardware to strip the CRC if we aren't connected to some kind of offboard management. We'll get there in steps.

So, as of 2.6.16.13, is the hardware stripping (SERC) enabled? Could you also let me know where this bit is defined in case I want to twiddle it myself (a quick grep for SERC in 2.6.16.13 yields nothing.)

Thanks,
Ben

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#3 lore raw
Quoted 3 lines · Ben Greear

So, as of 2.6.16.13, is the hardware stripping (SERC) enabled? Could you also let me know where this bit is defined in case I want to twiddle it myself (a quick grep for SERC in 2.6.16.13 yields nothing.)

You missed a C, it's SECRC (Strip Ethernet CRC) in the RCTL register or E1000_RCTL_SECRC.

- Chris

#4 lore raw
In reply to Ben Greear
Quoted 19 lines · Ben Greear
Quoted 18 lines · Jesse Brandeburg
Quoted 6 lines · Ben Greear

In commit: a292ca6efbc1f259ddfb9c902367f2588e0e8b0f to e1000_main.c, there is the change below.

I am curious why the skb_put no longer subtracts ETHERNET_FCS_SIZE from the length. Is the idea that we will now always include the FCS at the end of the skb?

This is a long and hairy story behind this, but there is a bit called SECRC that controls hardware stripping of the CRC. In *this* driver we turned that bit on, so that we didn't have to mess with skb->len -= 4 and the nasty negative unwrap if we were using multiple descriptors for rx.

Since then, we've removed multiple descriptor rx.

Quoted 8 lines
Quoted 5 lines

And after that, I've discovered that some BMCs are very unhappy if we strip CRCs using SECRC.

So, my related question is: is it okay if we just always leave the CRC on the end of the data? It doesn't seem to harm anything.

I believe it might mess up bridging logic if it tries to send the entire skb to a peer interface, ie cause an extra 4 bytes to be written.

hm, good point, we'll look into that.

Quoted 6 lines

My personal preference is to set a flag in the skb struct indicating whether or not the crc is appended (and skb_put). Then, bridging code can ignore it if needed, and sniffers and such can get the CRC in user-land. To remain backwards compat, at least the skb-put of the crc logic should default to OFF so that we don't break any existing user-land bridging logic. I have the ethtool API logic written to twiddle this save-crc behaviour if someone decides this is worthy of the kernel.

that seems like an excellent idea, however, finding room in the skb struct is fun.

Quoted 7 lines
Quoted 3 lines

Well, its a changing picture. I had planned to eventually enable the hardware to strip the CRC if we aren't connected to some kind of offboard management. We'll get there in steps.

So, as of 2.6.16.13, is the hardware stripping (SERC) enabled? Could you also let me know where this bit is defined in case I want to twiddle it myself (a quick grep for SERC in 2.6.16.13 yields nothing.)

Yes, SECRC is enabled in 2.6.16, for both packet split and legacy receive paths. It will probably stay enabled for 2.6.17 too unless the BMC communication bug is rated important enough for the change to be made. Unfortunately right now due to SECRC bit being set BMC communication over SMBUS is likely broken.

Jesse

author
#5 lore raw
Quoted 10 lines · Jesse Brandeburg
Quoted 6 lines

My personal preference is to set a flag in the skb struct indicating whether or not the crc is appended (and skb_put). Then, bridging code can ignore it if needed, and sniffers and such can get the CRC in user-land. To remain backwards compat, at least the skb-put of the crc logic should default to OFF so that we don't break any existing user-land bridging logic. I have the ethtool API logic written to twiddle this save-crc behaviour if someone decides this is worthy of the kernel.

that seems like an excellent idea, however, finding room in the skb struct is fun.

This field has 2 bits free. We only need one bit for this feature.

	__u8			pkt_type:3,
				fclone:2,
				ipvs_property:1;
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines

Well, its a changing picture. I had planned to eventually enable the hardware to strip the CRC if we aren't connected to some kind of offboard management. We'll get there in steps.

So, as of 2.6.16.13, is the hardware stripping (SERC) enabled? Could you also let me know where this bit is defined in case I want to twiddle it myself (a quick grep for SERC in 2.6.16.13 yields nothing.)

Quoted 5 lines

Yes, SECRC is enabled in 2.6.16, for both packet split and legacy receive paths. It will probably stay enabled for 2.6.17 too unless the BMC communication bug is rated important enough for the change to be made. Unfortunately right now due to SECRC bit being set BMC communication over SMBUS is likely broken.

Ok, thanks for the info.

Ben

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com