Re: [net PATCH] net: virtio: cap mtu when XDP programs are running

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jan 09, 2017 at 08:25:43PM -0800, John Fastabend wrote:
> On 17-01-09 07:55 PM, Michael S. Tsirkin wrote:
> > On Mon, Jan 09, 2017 at 07:30:34PM -0800, John Fastabend wrote:
> >> On 17-01-09 06:51 PM, Michael S. Tsirkin wrote:
> >>> On Tue, Jan 10, 2017 at 10:29:39AM +0800, Jason Wang wrote:
> >>>>
> >>>>
> >>>> On 2017年01月10日 07:58, Michael S. Tsirkin wrote:
> >>>>> On Mon, Jan 09, 2017 at 03:49:27PM -0800, John Fastabend wrote:
> >>>>>> On 17-01-09 03:24 PM, Michael S. Tsirkin wrote:
> >>>>>>> On Mon, Jan 09, 2017 at 03:13:15PM -0800, John Fastabend wrote:
> >>>>>>>> On 17-01-09 03:05 PM, Michael S. Tsirkin wrote:
> >>>>>>>>> On Thu, Jan 05, 2017 at 11:09:14AM +0800, Jason Wang wrote:
> >>>>>>>>>> On 2017年01月05日 02:57, John Fastabend wrote:
> >>>>>>>>>>> [...]
> >>>>>>>>>>>
> >>>>>>>>>>>> On 2017年01月04日 00:48, John Fastabend wrote:
> >>>>>>>>>>>>> On 17-01-02 10:14 PM, Jason Wang wrote:
> >>>>>>>>>>>>>> On 2017年01月03日 06:30, John Fastabend wrote:
> >>>>>>>>>>>>>>> XDP programs can not consume multiple pages so we cap the MTU to
> >>>>>>>>>>>>>>> avoid this case. Virtio-net however only checks the MTU at XDP
> >>>>>>>>>>>>>>> program load and does not block MTU changes after the program
> >>>>>>>>>>>>>>> has loaded.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> This patch sets/clears the max_mtu value at XDP load/unload time.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
> >>>>>>>>>>>>>>> ---
> >>>>>>>>>>> [...]
> >>>>>>>>>>>
> >>>>>>>>>>>>> OK so this logic is a bit too simply. When it resets the max_mtu I guess it
> >>>>>>>>>>>>> needs to read the mtu via
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>       virtio_cread16(vdev, ...)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> or we may break the negotiated mtu.
> >>>>>>>>>>>> Yes, this is a problem (even use ETH_MAX_MTU). We may need a method to notify
> >>>>>>>>>>>> the device about the mtu in this case which is not supported by virtio now.
> >>>>>>>>>>> Note this is not really a XDP specific problem. The guest can change the MTU
> >>>>>>>>>>> after init time even without XDP which I assume should ideally result in a
> >>>>>>>>>>> notification if the MTU is negotiated.
> >>>>>>>>>> Yes, Michael, do you think we need add some mechanism to notify host about
> >>>>>>>>>> MTU change in this case?
> >>>>>>>>>>
> >>>>>>>>>> Thanks
> >>>>>>>>> Why does host care?
> >>>>>>>>>
> >>>>>>>> Well the guest will drop packets after mtu has been reduced.
> >>>>>>> I didn't know. What place in code does this?
> >>>>>>>
> >>>>>> hmm in many of the drivers it is convention to use the mtu to set the rx
> >>>>>> buffer sizes and a receive side max length filter. For example in the Intel
> >>>>>> drivers if a packet with length greater than MTU + some headroom is received we
> >>>>>> drop it. I guess in the networking stack RX path though nothing forces this and
> >>>>>> virtio doesn't have any code to drop packets on rx size.
> >>>>>>
> >>>>>> In virtio I don't see any existing case currently. In the XDP case though we
> >>>>>> need to ensure packets fit in a page for the time being which is why I was
> >>>>>> looking at this code and generated this patch.
> >>>>> I'd say just look at the hardware max mtu. Ignore the configured mtu.
> >>>>>
> >>>>>
> >>>>
> >>>> Does this work for small buffers consider it always allocate skb with size
> >>>> of GOOD_PACKET_LEN?
> >>>
> >>> Spec says hardware won't send in packets > max mtu in config space.
> >>>
> >>>> I think in any case, we should limit max_mtu to
> >>>> GOOD_PACKET_LEN for small buffers.
> >>>>
> >>>> Thanks
> >>>
> >>> XDP seems to have a bunch of weird restrictions, I just
> >>> do not like it that the logic spills out to all drivers.
> >>> What if someone decides to extend it to two pages in the future?
> >>> Recode it all in all drivers ...
> >>>
> >>> Why can't net core enforce mtu?
> >>>
> >>
> >> OK I agree I'll put most the logic in rtnetlink.c when the program is added
> >> or removed.
> >>
> >> But, I'm looking at the non-XDP receive_small path now and wondering how does
> >> multiple buffer receives work (e.g. packet larger than GOOD_PACKET_LEN?)
> > 
> > I don't understand the question. Look at add_recvbuf_small,
> > it adds a tiny buffer for head and then the skb.
> > 
> 
> Specifically this seems to fail with mergeable buffers disabled
> 
> On the host:
> 
> # ip link set dev tap0 mtu 9000
> # ping 22.2 -s 2048
> 
> On the guest:
> 
> # insmod ./drivers/net/virtio_net.ko
> # ip link set dev eth0 mtu 9000

Why would it work? You are sending a packet larger than ethernet MTU.

> With mergeable buffers enabled no problems it works as I expect at least.

We don't expect to get these packets but
mergeable is able to process them anyway.
It's an accident :) 

> 
> > 
> >> I think
> >> this is what Jason is looking at as well? The mergeable case clearly looks at
> >> num_bufs in the descriptor to construct multi-buffer packets but nothing like
> >> that exists in the small_receive path as best I can tell.
> >>
> >> .John
> > 
> > There's always a single buffer there.
> > BTW it was always a legacy path but if it's now important for people we
> > should probably check ANY_LAYOUT and put header linearly with the packet
> > if there.
> >