| Message ID | 1459544811-24879-2-git-send-email-jakub.kicinski@netronome.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jakub Kicinski <jakub.kicinski@netronome.com> Date: Fri, 1 Apr 2016 22:06:37 +0100 > When calculating the RX buffer length we need to account for > up to 2 VLAN tags and up to 8 MPLS labels. Rounding up to 1k > is an relic of a distant past and can be removed. While at > it also remove trivial print statement. > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> I disagree with the MPLS aspect of this change. VLAN is special, in that when the hardware supports VLAN properly, the VLAN header doesn't eat into the MTU and is sort of "transparent". But MPLS doesn't work that way. MPLS is in the main frame and takes up MTU space. Therefore I see no reason to increase the buffer length by 8 * MPLS which is just a rediculous amount of wasted space. I'm not applying this without at least some more explanations about why exactly you need to account for these values in the commit message.
On Tue, 05 Apr 2016 11:39:17 -0400 (EDT), David Miller wrote: > From: Jakub Kicinski <jakub.kicinski@netronome.com> > Date: Fri, 1 Apr 2016 22:06:37 +0100 > > > When calculating the RX buffer length we need to account for > > up to 2 VLAN tags and up to 8 MPLS labels. Rounding up to 1k > > is an relic of a distant past and can be removed. While at > > it also remove trivial print statement. > > > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > I disagree with the MPLS aspect of this change. > > VLAN is special, in that when the hardware supports VLAN properly, the > VLAN header doesn't eat into the MTU and is sort of "transparent". > > But MPLS doesn't work that way. > > MPLS is in the main frame and takes up MTU space. Makes sense. RFC3032 counts MPLS label stack as Frame Payload. > Therefore I see no reason to increase the buffer length by 8 * MPLS > which is just a rediculous amount of wasted space. > > I'm not applying this without at least some more explanations about > why exactly you need to account for these values in the commit message. It's just what FW guys asked me for. I'll try to find out what their reasoning was. I have a patch queued up which gets rid of unconditionally reserving 64B for firmware prepend, I guess that made me too excited to pay attention to the fact the accounting for MPLS is indeed questionable...
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c index 43c618bafdb6..307c02c4ba4a 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c @@ -61,6 +61,7 @@ #include <linux/ktime.h> +#include <net/mpls.h> #include <net/vxlan.h> #include "nfp_net_ctrl.h" @@ -1911,9 +1912,6 @@ static void nfp_net_set_rx_mode(struct net_device *netdev) static int nfp_net_change_mtu(struct net_device *netdev, int new_mtu) { struct nfp_net *nn = netdev_priv(netdev); - u32 tmp; - - nn_dbg(nn, "New MTU = %d\n", new_mtu); if (new_mtu < 68 || new_mtu > nn->max_mtu) { nn_err(nn, "New MTU (%d) is not valid\n", new_mtu); @@ -1921,10 +1919,8 @@ static int nfp_net_change_mtu(struct net_device *netdev, int new_mtu) } netdev->mtu = new_mtu; - - /* Freelist buffer size rounded up to the nearest 1K */ - tmp = new_mtu + ETH_HLEN + VLAN_HLEN + NFP_NET_MAX_PREPEND; - nn->fl_bufsz = roundup(tmp, 1024); + nn->fl_bufsz = NFP_NET_MAX_PREPEND + ETH_HLEN + VLAN_HLEN * 2 + + MPLS_HLEN * 8 + new_mtu; /* restart if running */ if (netif_running(netdev)) {
When calculating the RX buffer length we need to account for up to 2 VLAN tags and up to 8 MPLS labels. Rounding up to 1k is an relic of a distant past and can be removed. While at it also remove trivial print statement. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)