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

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

[John Fastabend]

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>
---
 drivers/net/virtio_net.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 5deeda6..783e842 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1699,6 +1699,9 @@ static void virtnet_init_settings(struct net_device *dev)
 	.set_settings = virtnet_set_settings,
 };
 
+#define MIN_MTU ETH_MIN_MTU
+#define MAX_MTU ETH_MAX_MTU
+
 static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 {
 	unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr);
@@ -1748,6 +1751,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 			virtnet_set_queues(vi, curr_qp);
 			return PTR_ERR(prog);
 		}
+		dev->max_mtu = max_sz;
+	} else {
+		dev->max_mtu = ETH_MAX_MTU;
 	}
 
 	vi->xdp_queue_pairs = xdp_qp;
@@ -2133,9 +2139,6 @@ static bool virtnet_validate_features(struct virtio_device *vdev)
 	return true;
 }
 
-#define MIN_MTU ETH_MIN_MTU
-#define MAX_MTU ETH_MAX_MTU
-
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int i, err;

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

[Jason Wang]

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>
> ---
>   drivers/net/virtio_net.c |    9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 5deeda6..783e842 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1699,6 +1699,9 @@ static void virtnet_init_settings(struct net_device *dev)
>   	.set_settings = virtnet_set_settings,
>   };
>   
> +#define MIN_MTU ETH_MIN_MTU
> +#define MAX_MTU ETH_MAX_MTU
> +
>   static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>   {
>   	unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr);
> @@ -1748,6 +1751,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>   			virtnet_set_queues(vi, curr_qp);
>   			return PTR_ERR(prog);
>   		}
> +		dev->max_mtu = max_sz;
> +	} else {
> +		dev->max_mtu = ETH_MAX_MTU;

Or use ETH_DATA_LEN here consider we only allocate a size of 
GOOD_PACKET_LEN for each small buffer?

Thanks

>   	}
>   
>   	vi->xdp_queue_pairs = xdp_qp;
> @@ -2133,9 +2139,6 @@ static bool virtnet_validate_features(struct virtio_device *vdev)
>   	return true;
>   }
>   
> -#define MIN_MTU ETH_MIN_MTU
> -#define MAX_MTU ETH_MAX_MTU
> -
>   static int virtnet_probe(struct virtio_device *vdev)
>   {
>   	int i, err;
>

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

[John Fastabend]

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>
>> ---
>>   drivers/net/virtio_net.c |    9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 5deeda6..783e842 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1699,6 +1699,9 @@ static void virtnet_init_settings(struct net_device *dev)
>>       .set_settings = virtnet_set_settings,
>>   };
>>   +#define MIN_MTU ETH_MIN_MTU
>> +#define MAX_MTU ETH_MAX_MTU
>> +
>>   static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>>   {
>>       unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr);
>> @@ -1748,6 +1751,9 @@ static int virtnet_xdp_set(struct net_device *dev,
>> struct bpf_prog *prog)
>>               virtnet_set_queues(vi, curr_qp);
>>               return PTR_ERR(prog);
>>           }
>> +        dev->max_mtu = max_sz;
>> +    } else {
>> +        dev->max_mtu = ETH_MAX_MTU;
> 
> Or use ETH_DATA_LEN here consider we only allocate a size of GOOD_PACKET_LEN for
> each small buffer?
> 
> Thanks

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.

As for capping it at GOOD_PACKET_LEN this has the nice benefit of avoiding any
underestimates in EWMA predictions because it appears min estimates are capped
at GOOD_PACKET_LEN via get_mergeable_buf_len().

Thanks,
John

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

[John Fastabend]

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>
>> ---
>>   drivers/net/virtio_net.c |    9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 5deeda6..783e842 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1699,6 +1699,9 @@ static void virtnet_init_settings(struct net_device *dev)
>>       .set_settings = virtnet_set_settings,
>>   };
>>   +#define MIN_MTU ETH_MIN_MTU
>> +#define MAX_MTU ETH_MAX_MTU
>> +
>>   static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>>   {
>>       unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr);
>> @@ -1748,6 +1751,9 @@ static int virtnet_xdp_set(struct net_device *dev,
>> struct bpf_prog *prog)
>>               virtnet_set_queues(vi, curr_qp);
>>               return PTR_ERR(prog);
>>           }
>> +        dev->max_mtu = max_sz;
>> +    } else {
>> +        dev->max_mtu = ETH_MAX_MTU;
> 
> Or use ETH_DATA_LEN here consider we only allocate a size of GOOD_PACKET_LEN for
> each small buffer?
> 
> Thanks

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.

As for capping it at GOOD_PACKET_LEN this has the nice benefit of avoiding any
underestimates in EWMA predictions because it appears min estimates are capped
at GOOD_PACKET_LEN via get_mergeable_buf_len().

Thanks,
John

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

[Jason Wang]

  case.



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>
>>> ---
>>>    drivers/net/virtio_net.c |    9 ++++++---
>>>    1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 5deeda6..783e842 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -1699,6 +1699,9 @@ static void virtnet_init_settings(struct net_device *dev)
>>>        .set_settings = virtnet_set_settings,
>>>    };
>>>    +#define MIN_MTU ETH_MIN_MTU
>>> +#define MAX_MTU ETH_MAX_MTU
>>> +
>>>    static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>>>    {
>>>        unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr);
>>> @@ -1748,6 +1751,9 @@ static int virtnet_xdp_set(struct net_device *dev,
>>> struct bpf_prog *prog)
>>>                virtnet_set_queues(vi, curr_qp);
>>>                return PTR_ERR(prog);
>>>            }
>>> +        dev->max_mtu = max_sz;
>>> +    } else {
>>> +        dev->max_mtu = ETH_MAX_MTU;
>> Or use ETH_DATA_LEN here consider we only allocate a size of GOOD_PACKET_LEN for
>> each small buffer?
>>
>> Thanks
> 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.
>
> As for capping it at GOOD_PACKET_LEN this has the nice benefit of avoiding any
> underestimates in EWMA predictions because it appears min estimates are capped
> at GOOD_PACKET_LEN via get_mergeable_buf_len().

This seems something misunderstanding here, I meant only use 
GOOD_PACKET_LEN for small buffer (which does not use EWMA).

Thanks

>
> Thanks,
> John
>

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

[John Fastabend]

[...]

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

>>
>> As for capping it at GOOD_PACKET_LEN this has the nice benefit of avoiding any
>> underestimates in EWMA predictions because it appears min estimates are capped
>> at GOOD_PACKET_LEN via get_mergeable_buf_len().
> 
> This seems something misunderstanding here, I meant only use GOOD_PACKET_LEN for
> small buffer (which does not use EWMA).
> 

Yep I think its all cleared up now.

Thanks.

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

[Jason Wang]

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

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

[Michael S. Tsirkin]

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?

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

[John Fastabend]

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. Although the guest
by reducing its MTU in some sense must expect this. Likewise if the host were
to change MTU after virtio_net probe time the guest would not learn about it.

I think at best negotiating the mtu is just a hint? If system _really_ cares
we could use lldp or some other out of band mechanism to learn/set/adjust MTU
on both systems and it would be more robust. I'm not actually convinced this
is a problem in bare metal systems we have the same issue with physical
switches and solve it out of band via configuration, protocols, etc.

.John

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

[Michael S. Tsirkin]

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?

> Although the guest
> by reducing its MTU in some sense must expect this. Likewise if the host were
> to change MTU after virtio_net probe time the guest would not learn about it.

The spec explicitly disallows this last one.

> I think at best negotiating the mtu is just a hint? If system _really_ cares
> we could use lldp or some other out of band mechanism to learn/set/adjust MTU
> on both systems and it would be more robust. I'm not actually convinced this
> is a problem in bare metal systems we have the same issue with physical
> switches and solve it out of band via configuration, protocols, etc.
> 
> .John

ATM we don't have negotiation in virtio, just a max mtu limit.
This doesn't free guest from configuring mtu correctly,
just helps it avoid doing something clearly bogus.

-- 
MST

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

[John Fastabend]

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.

>> Although the guest
>> by reducing its MTU in some sense must expect this. Likewise if the host were
>> to change MTU after virtio_net probe time the guest would not learn about it.
> 
> The spec explicitly disallows this last one.

OK. By the way were do I get the latest source I see the published virtio1.0 at
the oasis-open.org site but it doesn't mention the MTU logic.

> 
>> I think at best negotiating the mtu is just a hint? If system _really_ cares
>> we could use lldp or some other out of band mechanism to learn/set/adjust MTU
>> on both systems and it would be more robust. I'm not actually convinced this
>> is a problem in bare metal systems we have the same issue with physical
>> switches and solve it out of band via configuration, protocols, etc.
>>
>> .John
> 
> ATM we don't have negotiation in virtio, just a max mtu limit.
> This doesn't free guest from configuring mtu correctly,
> just helps it avoid doing something clearly bogus.
> 

Yep. I'm fine with calling it a misconfiguration if the guest reduces the MTU
and the host continues to send packets @ advertised MTU.

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

[Michael S. Tsirkin]

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.


> >> Although the guest
> >> by reducing its MTU in some sense must expect this. Likewise if the host were
> >> to change MTU after virtio_net probe time the guest would not learn about it.
> > 
> > The spec explicitly disallows this last one.
> 
> OK. By the way were do I get the latest source I see the published virtio1.0 at
> the oasis-open.org site but it doesn't mention the MTU logic.

You need to get it from svn.

> > 
> >> I think at best negotiating the mtu is just a hint? If system _really_ cares
> >> we could use lldp or some other out of band mechanism to learn/set/adjust MTU
> >> on both systems and it would be more robust. I'm not actually convinced this
> >> is a problem in bare metal systems we have the same issue with physical
> >> switches and solve it out of band via configuration, protocols, etc.
> >>
> >> .John
> > 
> > ATM we don't have negotiation in virtio, just a max mtu limit.
> > This doesn't free guest from configuring mtu correctly,
> > just helps it avoid doing something clearly bogus.
> > 
> 
> Yep. I'm fine with calling it a misconfiguration if the guest reduces the MTU
> and the host continues to send packets @ advertised MTU.

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

[Jason Wang]

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? I think in any case, we should limit max_mtu to 
GOOD_PACKET_LEN for small buffers.

Thanks

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

[Michael S. Tsirkin]

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?

-- 
MST

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

[John Fastabend]

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

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

[Jason Wang]

On 2017年01月10日 10:51, 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.

Yes, but if max mtu is greater than GOOD_PACKET_LEN, packet will be dropped.

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

Not sure it's a good idea to change mtu silently without notifying user.

Thanks

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

[Michael S. Tsirkin]

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

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

[John Fastabend]

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

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


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

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

[Michael S. Tsirkin]

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

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

[Michael S. Tsirkin]

On Tue, Jan 10, 2017 at 04:51:34AM +0200, Michael S. Tsirkin wrote:
> 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?

And BTW limits on MTU are a problem that will have to be
addressed sooner or later, disabling offloads on the NIC
is one thing, but reconfiguring all of the network
with a lower MTU is another.


> -- 
> MST

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

[Jason Wang]

On 2017年01月10日 13:00, Michael S. Tsirkin wrote:
> 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.

Ok, does it mean virtio-net does not support Jumbo frame? And if it 
can't work, use MAX_MTU as max_mtu is a bug to me.

>
>> 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:)  
>

But path MTU discovery indeed benefits from this "accident".

Thanks