All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jarod Wilson <jarod@redhat.com>
To: David Miller <davem@davemloft.net>
Cc: kubakici@wp.pl, linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH v3 net-next 2/2] net: deprecate eth_change_mtu, remove usage
Date: Mon, 17 Oct 2016 16:22:28 -0400	[thread overview]
Message-ID: <20161017202228.GO14983@redhat.com> (raw)
In-Reply-To: <20161017200712.GN14983@redhat.com>

On Mon, Oct 17, 2016 at 04:07:12PM -0400, Jarod Wilson wrote:
> On Mon, Oct 17, 2016 at 01:25:53PM -0400, David Miller wrote:
> > From: Jakub Kicinski <kubakici@wp.pl>
> > Date: Mon, 17 Oct 2016 18:20:49 +0100
> > 
> > > Hm.  I must be missing something really obvious.  I just booted
> > > net-next an hour ago and couldn't set MTU to anything larger than 1500
> > > on either nfp or igb.  As far as I can read the code it will set the
> > > max_mtu to 1500 in setup_ether() but none of the jumbo-capable drivers
> > > had been touched by Jarod so far...
> > 
> > Indeed.
> > 
> > Jarod, this doesn't work.
> > 
> > I guess the idea was that if the driver overrides ndo_change_mtu and
> > enforeced it's limits there, the driver would still work after your
> > changes.
> > 
> > But that isn't what is happening, look at the IGB example.
> > 
> > It uses ether_setup(), which sets those new defaults, but now when
> > the MTU is changed you enforce those default min/max before the
> > driver's ->ndo_change_mtu() has a change to step in front and make
> > the decision on it's own.
> > 
> > This means your changes pretty much did indeed break a lot of
> > drivers's ability to set larger than a 1500 byte MTU.
> 
> Argh. Yeah, I see it now. I was primarily operating with the follow-on
> patches also in play, which do touch all the ethernet drivers and set
> max_mtu to match current behavior, didn't consider the max_mtu case where
> only the initial patches were applied and the follow-on ones weren't. I've
> sent that set, which should theoretically make this problem go away, but I
> can also try to rework things if need be to restore intermediate jumbo
> frames functionality. (And there are actually non-ethernet devices that
> also call ether_setup and may or may not have larger than 1500 mtu that
> aren't yet addressed by that follow-on set).

Looks like the simplest thing to do is going to be to revert a52ad514, and
only make that change after all callers of ether_setup() are setting
min/max_mtu themselves as needed, then it can be reintroduced.

-- 
Jarod Wilson
jarod@redhat.com

  reply	other threads:[~2016-10-17 20:23 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-12 20:11 [PATCH net-next 0/2] net: centralize net_device MTU bounds checking Jarod Wilson
2016-09-12 20:11 ` [PATCH net-next 1/2] net: centralize net_device min/max MTU checking Jarod Wilson
2016-09-12 20:11 ` [PATCH net-next 2/2] net: deprecate eth_change_mtu, remove usage Jarod Wilson
2016-09-13 21:59   ` kbuild test robot
2016-09-14 18:45     ` Jarod Wilson
2016-09-28 21:17 ` [PATCH v2 net-next 0/2] net: centralize net_device MTU bounds checking Jarod Wilson
2016-09-28 21:17   ` [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking Jarod Wilson
2016-09-28 21:17   ` [PATCH v2 net-next 2/2] net: deprecate eth_change_mtu, remove usage Jarod Wilson
2016-09-28 22:20 ` [PATCH v2 net-next 0/2] net: centralize net_device MTU bounds checking Jarod Wilson
2016-09-28 22:20   ` [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking Jarod Wilson
2016-09-30  9:37     ` Jakub Sitnicki
2016-10-03  2:43       ` David Miller
2016-10-03 17:46         ` Jarod Wilson
2016-10-08  2:04         ` [PATCH v3 net-next 0/2] net: centralize net_device MTU bounds checking Jarod Wilson
2016-10-08  2:04           ` [PATCH v3 net-next 1/2] net: centralize net_device min/max MTU checking Jarod Wilson
2016-10-08  2:04           ` [PATCH v3 net-next 2/2] net: deprecate eth_change_mtu, remove usage Jarod Wilson
2016-10-17 16:20             ` Jakub Kicinski
2016-10-17 16:49               ` David Miller
2016-10-17 17:00                 ` Jakub Kicinski
2016-10-17 17:04                   ` Jakub Kicinski
2016-10-17 17:15                   ` David Miller
2016-10-17 17:20                     ` Jakub Kicinski
2016-10-17 17:25                       ` David Miller
2016-10-17 20:07                         ` Jarod Wilson
2016-10-17 20:22                           ` Jarod Wilson [this message]
2016-10-13 13:37           ` [PATCH v3 net-next 0/2] net: centralize net_device MTU bounds checking David Miller
2016-09-28 22:20   ` [PATCH v2 net-next 2/2] net: deprecate eth_change_mtu, remove usage Jarod Wilson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161017202228.GO14983@redhat.com \
    --to=jarod@redhat.com \
    --cc=davem@davemloft.net \
    --cc=kubakici@wp.pl \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.