| Message ID | 1381838188-9625-2-git-send-email-sathya.perla@emulex.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 15-10-2013 15:56, Sathya Perla wrote: > From: Vasundhara Volam <vasundhara.volam@emulex.com> > Pktgen can generate non-TSO frames of arbitrary length disregarding > the MTU value of the physical interface. Drop such frames in the driver > instead of sending them to HW as it cannot handle such frames. > Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com> > Signed-off-by: Sathya Perla <sathya.perla@emulex.com> > --- > drivers/net/ethernet/emulex/benet/be_main.c | 9 +++++++-- > 1 files changed, 7 insertions(+), 2 deletions(-) > diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c > index 2c38cc4..76057b8 100644 > --- a/drivers/net/ethernet/emulex/benet/be_main.c > +++ b/drivers/net/ethernet/emulex/benet/be_main.c > @@ -855,6 +855,13 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter, > unsigned int eth_hdr_len; > struct iphdr *ip; > > + /* Don't allow non-TSO packets longer than MTU */ > + eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ? > + VLAN_ETH_HLEN : ETH_HLEN; > + if (!skb_is_gso(skb) && > + (skb->len - eth_hdr_len) > adapter->netdev->mtu) > + goto tx_drop; This *goto* is indented one tab too much. WBR, Sergei -- 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
On Tue, 2013-10-15 at 17:26 +0530, Sathya Perla wrote: > From: Vasundhara Volam <vasundhara.volam@emulex.com> > > Pktgen can generate non-TSO frames of arbitrary length disregarding > the MTU value of the physical interface. Drop such frames in the driver > instead of sending them to HW as it cannot handle such frames. > > Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com> > Signed-off-by: Sathya Perla <sathya.perla@emulex.com> > --- > drivers/net/ethernet/emulex/benet/be_main.c | 9 +++++++-- > 1 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c > index 2c38cc4..76057b8 100644 > --- a/drivers/net/ethernet/emulex/benet/be_main.c > +++ b/drivers/net/ethernet/emulex/benet/be_main.c > @@ -855,6 +855,13 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter, > unsigned int eth_hdr_len; > struct iphdr *ip; > > + /* Don't allow non-TSO packets longer than MTU */ > + eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ? > + VLAN_ETH_HLEN : ETH_HLEN; > + if (!skb_is_gso(skb) && > + (skb->len - eth_hdr_len) > adapter->netdev->mtu) > + goto tx_drop; > + When you say 'cannot handle them', is it some kind of nasty thing like hang / crash ? One could imagine gso_size + sizeof(headers) > mtu, and give the same problem ? AFAIK, we have no check in net/core/dev.c. Maybe we should instead add them there (and in pktgen) -- 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
On 10/15/2013 03:47 PM, Eric Dumazet wrote: > On Tue, 2013-10-15 at 17:26 +0530, Sathya Perla wrote: >> From: Vasundhara Volam <vasundhara.volam@emulex.com> >> >> Pktgen can generate non-TSO frames of arbitrary length disregarding >> the MTU value of the physical interface. Drop such frames in the driver >> instead of sending them to HW as it cannot handle such frames. >> >> Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com> >> Signed-off-by: Sathya Perla <sathya.perla@emulex.com> >> --- >> drivers/net/ethernet/emulex/benet/be_main.c | 9 +++++++-- >> 1 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c >> index 2c38cc4..76057b8 100644 >> --- a/drivers/net/ethernet/emulex/benet/be_main.c >> +++ b/drivers/net/ethernet/emulex/benet/be_main.c >> @@ -855,6 +855,13 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter, >> unsigned int eth_hdr_len; >> struct iphdr *ip; >> >> + /* Don't allow non-TSO packets longer than MTU */ >> + eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ? >> + VLAN_ETH_HLEN : ETH_HLEN; >> + if (!skb_is_gso(skb) && >> + (skb->len - eth_hdr_len) > adapter->netdev->mtu) >> + goto tx_drop; >> + > > When you say 'cannot handle them', is it some kind of nasty thing like > hang / crash ? AFAIK, the firmware in the card becomes unresponsive and reboot is needed to make the NIC working. Ivan -- 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
> -----Original Message----- > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > > > > Pktgen can generate non-TSO frames of arbitrary length disregarding > > the MTU value of the physical interface. Drop such frames in the driver > > instead of sending them to HW as it cannot handle such frames. > > > > Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com> > > Signed-off-by: Sathya Perla <sathya.perla@emulex.com> > > --- > > drivers/net/ethernet/emulex/benet/be_main.c | 9 +++++++-- > > 1 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/emulex/benet/be_main.c > b/drivers/net/ethernet/emulex/benet/be_main.c > > index 2c38cc4..76057b8 100644 > > --- a/drivers/net/ethernet/emulex/benet/be_main.c > > +++ b/drivers/net/ethernet/emulex/benet/be_main.c > > @@ -855,6 +855,13 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter > *adapter, > > unsigned int eth_hdr_len; > > struct iphdr *ip; > > > > + /* Don't allow non-TSO packets longer than MTU */ > > + eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ? > > + VLAN_ETH_HLEN : ETH_HLEN; > > + if (!skb_is_gso(skb) && > > + (skb->len - eth_hdr_len) > adapter->netdev->mtu) > > + goto tx_drop; > > + > > When you say 'cannot handle them', is it some kind of nasty thing like > hang / crash ? This chip goes into an unrecoverable error state (needing a server reboot to rectify.) > > One could imagine gso_size + sizeof(headers) > mtu, and give the same > problem ? Won't the mss (gso_size) value for TCP pkts be derived from the mtu of the out-going interface? In that case when will gso_size + headers > mtu? In any case, for TSO pkts, the HW just drops pkts when frames after segmentation are longer than 9018bytes (or in recent FW versions 9200b) > > > AFAIK, we have no check in net/core/dev.c. > > Maybe we should instead add them there (and in pktgen) I agree; doing this will solve issues with any other drivers/devices if they have a similar problem. thanks, -Sathya
On Wed, 2013-10-16 at 12:08 +0000, Sathya Perla wrote: > Won't the mss (gso_size) value for TCP pkts be derived from the mtu of the out-going interface? > In that case when will gso_size + headers > mtu? Not on forwarding :( Let see we have two ethernet devices, with different mtu : 1500 and 1400 GRO could aggregate GRO packets with MSS=1460 (gso_size=1460), then we hapilly send this GSO packet to the other device, and apparently we have no check against the lower mtu. In the best case GRO packet is lost, but in some other cases, the device hangs... > In any case, for TSO pkts, the HW just drops pkts when frames after segmentation > are longer than 9018bytes (or in recent FW versions 9200b) -- 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
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c index 2c38cc4..76057b8 100644 --- a/drivers/net/ethernet/emulex/benet/be_main.c +++ b/drivers/net/ethernet/emulex/benet/be_main.c @@ -855,6 +855,13 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter, unsigned int eth_hdr_len; struct iphdr *ip; + /* Don't allow non-TSO packets longer than MTU */ + eth_hdr_len = (ntohs(skb->protocol) == ETH_P_8021Q) ? + VLAN_ETH_HLEN : ETH_HLEN; + if (!skb_is_gso(skb) && + (skb->len - eth_hdr_len) > adapter->netdev->mtu) + goto tx_drop; + /* Lancer, SH-R ASICs have a bug wherein Packets that are 32 bytes or less * may cause a transmit stall on that port. So the work-around is to * pad short packets (<= 32 bytes) to a 36-byte length. @@ -869,8 +876,6 @@ static struct sk_buff *be_xmit_workarounds(struct be_adapter *adapter, * incorrecly when VLAN tag is inserted by HW. * For padded packets, Lancer computes incorrect checksum. */ - eth_hdr_len = ntohs(skb->protocol) == ETH_P_8021Q ? - VLAN_ETH_HLEN : ETH_HLEN; if (skb->len <= 60 && (lancer_chip(adapter) || vlan_tx_tag_present(skb)) && is_ipv4_pkt(skb)) {