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

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

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.


> 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.

-- 
MST