Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

2022-08-10 Thread Si-Wei Liu




On 8/9/2022 11:15 PM, Michael S. Tsirkin wrote:

On Wed, Aug 10, 2022 at 02:14:07AM -0400, Michael S. Tsirkin wrote:

On Tue, Aug 09, 2022 at 04:24:23PM -0700, Si-Wei Liu wrote:


On 8/9/2022 3:49 PM, Parav Pandit wrote:

From: Michael S. Tsirkin 
Sent: Tuesday, August 9, 2022 6:26 PM
To: Parav Pandit 
Cc: Si-Wei Liu ; Jason Wang
; Gavin Li ; Hemminger,
Stephen ; davem
; virtualization ; Virtio-Dev ;
jesse.brandeb...@intel.com; alexander.h.du...@intel.com;
kubak...@wp.pl; sridhar.samudr...@intel.com; losewe...@gmail.com; Gavi
Teitz 
Subject: Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for
big packets

On Tue, Aug 09, 2022 at 09:49:03PM +, Parav Pandit wrote:

From: Michael S. Tsirkin 
Sent: Tuesday, August 9, 2022 5:38 PM

[..]

I think virtio-net driver doesn't differentiate MTU and MRU, in
which case the receive buffer will be reduced to fit the 1500B
payload size when mtu is lowered down to 1500 from 9000.

How? Driver reduced the mXu to 1500, say it is improved to post
buffers of

1500 bytes.

Device doesn't know about it because mtu in config space is RO field.
Device keep dropping 9K packets because buffers posted are 1500

bytes.

This is because device follows the spec " The device MUST NOT pass

received packets that exceed mtu".


The "mtu" here is the device config field, which is

  /* Default maximum transmit unit advice */


It is the field from struct virtio_net_config.mtu. right?
This is RO field for driver.


there is no guarantee device will not get a bigger packet.

Right. That is what I also hinted.
Hence, allocating buffers worth upto mtu is safer.

yes


When user overrides it, driver can be further optimized to honor such new

value on rx buffer posting.

no, not without a feature bit promising device won't get wedged.


I mean to say as_it_stands today, driver can decide to post smaller buffers 
with larger mtu.
Why device should be affected with it?
( I am not proposing such weird configuration but asking for sake of 
correctness).

I am also confused how the device can be wedged in this case.

Yea sorry. I misunderstood the code. It can't be.

Here's a problem as I see it. Let's say we reduce mtu.
Small buffers are added. Now we increase mtu.
Device will drop all packets until small buffers are consumed.

Should we make this depend on the vq reset ability maybe?
To be honest I am not sure if worth it, very few user changes MTU on the 
fly with traffic ongoing, for the most cases I've seen users just change 
it only once in deployment time. Even if they change it on the fly they 
may need to be aware of the consequence and implication of loss of 
packets. In real devices, mtu change could end up with link status 
change and in that case there's usually no guarantee arrived  packet 
will be kept during the window.


While I could understand this would slightly introduce regression on 
functionality, as the worst case for packet loss if device dropping 
packets, it would be all smaller buffers of the full queue size. For 
correctness and elegance I don't mind introducing specific feature that 
changes MTU, or even relying on vq_reset is fine. device_reset would be 
too overwhelmed for this special use case IMHO.


-Siwei




--
MST



-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



Re: [virtio-dev] Re: [PATCH 1/4] content: Introduce driver/device auxiliary notifications

2022-08-10 Thread Michael S. Tsirkin
On Wed, Aug 10, 2022 at 07:41:08PM +0200, Halil Pasic wrote:
> On Wed, 10 Aug 2022 11:54:35 +0200
> Cornelia Huck  wrote:
> 
> > >> These device-specific notifications are needed later when adding support
> > >> for virtio-vhost-user device.
> > >> 
> > >> Signed-off-by: Usama Arif 
> > >> Signed-off-by: Stefan Hajnoczi 
> > >> Signed-off-by: Nikos Dragazis   
> > >
> > > I see ccw is missing. Cornelia, any suggestions?  
> > 
> > Hmm... I seem to be really behind on ccw things :(
> > 
> > We can probably use the following:
> > 
> > - for device->driver notification, use the next bit in the secondary
> >   indicators (bit 0 is used for config change notification)
> > - for driver->device notification, maybe use a new subcode for diagnose
> >   0x500 (4 is probably the next free one?)
> > 
> 
> Sounds reasonable! I will have to double check the DIAG stuff though. I'm
> not sure where what needs to be reserved and documented. 
> 
> > I have not looked at the requirements deeply, though.
> > 
> > This highlights another problem, however: When we introduce new features
> > that require a transport-specific implementation, we often end up with a
> > PCI implementation, but sometimes MMIO and more often ccw are left
> > behind -- which is understandable, as PCI is what most people use, and
> > ccw is something only a very few people are familiar with. This sadly
> > means that we have a backlog of features supported in PCI, but not in
> > ccw... requiring implementations for ccw would put an undue burden on
> > contributors, as most of them are unlikely to write anything for a
> > mainframe, ever. On the flip side, I do not have enough bandwith to deal
> > with all of this.
> 
> I'm completely with you in a sense that I see the same problem. I think
> we have to get these resolved on a case by case basis. In my opinion at
> least in theory it would make a big difference, whether the new feature
> obligatory or not. But since VIRTIO is big on compatibility, and also
> cares about the initial investment required, in practice, I think, we
> are mostly good with the transports delivering features on their own
> schedule. What I mean here is: it is kind of difficult to make a new
> facility (like shm, or aux notifications) mandatory, because stuff
> that conform to a previous incarnation of the spec would become
> non-conform.
> 
> And the people who care about the particular transport, and the users
> of the transport (indirectly also platforms) should make up their own
> mind with regards to whether and when to invest into the new facilities
> and the new tech and opportunities associated with those.
> 
> OTOH when reading the spec, it my strike one as strange, that for example
> CCW does not mention aux notifications at all. One idea: maybe we could
> add a note, or a subsection, or something, which states the list of
> general optional virtio facilities or features not supported by the given
> transport on the spec level for a given incarnation of the spec.

Yes I think each transport should list features it does not
support, and a feature specific to some transports must
also require that other transports disable it and
that drivers do not ack it.
Otherwise it's too easy for devices to offer the feature bit
by mistake.

> I think making the people not motivated to do the design and write the
> spec for all the platforms add to that list is a reasonable middle
> ground. It would also make the differences very clear, and the same goes
> for the intention (i.e. not omitted by mistake).
> 
> > 
> > Halil, any thoughts (on any of the above)?
> 
> Yes, definitely! See above. :) Thanks for drawing my attention to this.
> I'm very interested in your opinion.
> 
> Regards,
> Halil


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



Re: [virtio-dev] Re: [PATCH 1/4] content: Introduce driver/device auxiliary notifications

2022-08-10 Thread Halil Pasic
On Wed, 10 Aug 2022 08:45:25 -0400
"Michael S. Tsirkin"  wrote:

> On Wed, Aug 10, 2022 at 11:54:35AM +0200, Cornelia Huck wrote:
> > On Tue, Aug 09 2022, "Michael S. Tsirkin"  wrote:
> >   
> > > On Wed, Mar 30, 2022 at 04:21:02PM +0100, Usama Arif wrote:  
> > >> Driver auxiliary notifications allow the device to send notifications
> > >> other than configuration changes and used buffer notifications to the
> > >> driver, these are optional and their meaning is device-specific.
> > >> 
> > >> Device auxiliary notifcations allow the driver to send notifcations
> > >> other than available buffer notifications to the device for example
> > >> through a device register, these are optional and their meaning is
> > >> device-specific.
> > >> 
> > >> These device-specific notifications are needed later when adding support
> > >> for virtio-vhost-user device.
> > >> 
> > >> Signed-off-by: Usama Arif 
> > >> Signed-off-by: Stefan Hajnoczi 
> > >> Signed-off-by: Nikos Dragazis   
> > >
> > > I see ccw is missing. Cornelia, any suggestions?  
> > 
> > Hmm... I seem to be really behind on ccw things :(
> > 
> > We can probably use the following:
> > 
> > - for device->driver notification, use the next bit in the secondary
> >   indicators (bit 0 is used for config change notification)
> > - for driver->device notification, maybe use a new subcode for diagnose
> >   0x500 (4 is probably the next free one?)
> > 
> > I have not looked at the requirements deeply, though.
> > 
> > This highlights another problem, however: When we introduce new features
> > that require a transport-specific implementation, we often end up with a
> > PCI implementation, but sometimes MMIO and more often ccw are left
> > behind -- which is understandable, as PCI is what most people use, and
> > ccw is something only a very few people are familiar with. This sadly
> > means that we have a backlog of features supported in PCI, but not in
> > ccw... requiring implementations for ccw would put an undue burden on
> > contributors, as most of them are unlikely to write anything for a
> > mainframe, ever. On the flip side, I do not have enough bandwith to deal
> > with all of this.
> > 
> > Halil, any thoughts (on any of the above)?  
> 
> Kind of depends. We Do we want to add a "universal config"
> structure shared between transports?
> Will help with some use-cases though not this one.

I'm for "unversal config"! Regarding this use case I have to dig a
little deeper to really understand!

Regards,
Halil 

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



Re: [virtio-dev] Re: [PATCH 1/4] content: Introduce driver/device auxiliary notifications

2022-08-10 Thread Halil Pasic
On Wed, 10 Aug 2022 11:54:35 +0200
Cornelia Huck  wrote:

> >> These device-specific notifications are needed later when adding support
> >> for virtio-vhost-user device.
> >> 
> >> Signed-off-by: Usama Arif 
> >> Signed-off-by: Stefan Hajnoczi 
> >> Signed-off-by: Nikos Dragazis   
> >
> > I see ccw is missing. Cornelia, any suggestions?  
> 
> Hmm... I seem to be really behind on ccw things :(
> 
> We can probably use the following:
> 
> - for device->driver notification, use the next bit in the secondary
>   indicators (bit 0 is used for config change notification)
> - for driver->device notification, maybe use a new subcode for diagnose
>   0x500 (4 is probably the next free one?)
> 

Sounds reasonable! I will have to double check the DIAG stuff though. I'm
not sure where what needs to be reserved and documented. 

> I have not looked at the requirements deeply, though.
> 
> This highlights another problem, however: When we introduce new features
> that require a transport-specific implementation, we often end up with a
> PCI implementation, but sometimes MMIO and more often ccw are left
> behind -- which is understandable, as PCI is what most people use, and
> ccw is something only a very few people are familiar with. This sadly
> means that we have a backlog of features supported in PCI, but not in
> ccw... requiring implementations for ccw would put an undue burden on
> contributors, as most of them are unlikely to write anything for a
> mainframe, ever. On the flip side, I do not have enough bandwith to deal
> with all of this.

I'm completely with you in a sense that I see the same problem. I think
we have to get these resolved on a case by case basis. In my opinion at
least in theory it would make a big difference, whether the new feature
obligatory or not. But since VIRTIO is big on compatibility, and also
cares about the initial investment required, in practice, I think, we
are mostly good with the transports delivering features on their own
schedule. What I mean here is: it is kind of difficult to make a new
facility (like shm, or aux notifications) mandatory, because stuff
that conform to a previous incarnation of the spec would become
non-conform.

And the people who care about the particular transport, and the users
of the transport (indirectly also platforms) should make up their own
mind with regards to whether and when to invest into the new facilities
and the new tech and opportunities associated with those.

OTOH when reading the spec, it my strike one as strange, that for example
CCW does not mention aux notifications at all. One idea: maybe we could
add a note, or a subsection, or something, which states the list of
general optional virtio facilities or features not supported by the given
transport on the spec level for a given incarnation of the spec.

I think making the people not motivated to do the design and write the
spec for all the platforms add to that list is a reasonable middle
ground. It would also make the differences very clear, and the same goes
for the intention (i.e. not omitted by mistake).

> 
> Halil, any thoughts (on any of the above)?

Yes, definitely! See above. :) Thanks for drawing my attention to this.
I'm very interested in your opinion.

Regards,
Halil

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

2022-08-10 Thread Michael S. Tsirkin
On Wed, Aug 10, 2022 at 05:06:33PM +, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin 
> > Sent: Wednesday, August 10, 2022 12:59 PM
> > 
> > On Wed, Aug 10, 2022 at 04:22:41PM +, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin 
> > > > Sent: Wednesday, August 10, 2022 12:05 PM
> > > >
> > > > On Wed, Aug 10, 2022 at 04:00:08PM +, Parav Pandit wrote:
> > > > >
> > > > > > From: Michael S. Tsirkin 
> > > > > > Sent: Wednesday, August 10, 2022 5:03 AM
> > > > > > > >
> > > > > > > > Should we make this depend on the vq reset ability maybe?
> > > > > > >
> > > > > > > The advantage of this is to keep TX working. Or we can use
> > > > > > > device reset as a fallback if there's no vq reset.
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > > Device reset is really annoying in that it loses all the state:
> > > > > > rx filters etc etc.
> > > > >
> > > > > The elegant solution is let driver tell the new mtu to the device.
> > > > > One way to do so is by using existing ctrl vq.
> > > >
> > > > That will need a new feature bit.
> > > >
> > > Yes. ctrl vq can tell what all configuration does it allow. :) Or you
> > > prefer feature bit?
> > 
> > We did feature bits for this in the past.
> > 
> Ok. Will try to draft the update for future.
> 
> Gavin should repost the patch to address comments unrelated to this future 
> bit anyway.
> Right?

Right.

> > > > > If merged buffer is done, and new mtu is > minimum posting size,
> > > > > no need
> > > > to undergo vq reset.
> > > > > If merged buffer is not done, and buffer posted are smaller than
> > > > > new mtu,
> > > > undergo vq reset optionally.
> > > >
> > > > This can be done with or without sending mtu to device.
> > > Yes, telling mtu to device helps device to optimize and adhere to the spec
> > line " The device MUST NOT pass received packets that exceed mtu" in
> > section 5.1.4.1.
> > 
> > Again, that line refers to \field{mtu} which is the max mtu supported,
> > irrespective to anything driver does.
> > 
> > --
> > MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

2022-08-10 Thread Michael S. Tsirkin
On Wed, Aug 10, 2022 at 12:58:58PM -0400, Michael S. Tsirkin wrote:
> On Wed, Aug 10, 2022 at 04:22:41PM +, Parav Pandit wrote:
> > 
> > > From: Michael S. Tsirkin 
> > > Sent: Wednesday, August 10, 2022 12:05 PM
> > > 
> > > On Wed, Aug 10, 2022 at 04:00:08PM +, Parav Pandit wrote:
> > > >
> > > > > From: Michael S. Tsirkin 
> > > > > Sent: Wednesday, August 10, 2022 5:03 AM
> > > > > > >
> > > > > > > Should we make this depend on the vq reset ability maybe?
> > > > > >
> > > > > > The advantage of this is to keep TX working. Or we can use device
> > > > > > reset as a fallback if there's no vq reset.
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > Device reset is really annoying in that it loses all the state:
> > > > > rx filters etc etc.
> > > >
> > > > The elegant solution is let driver tell the new mtu to the device.
> > > > One way to do so is by using existing ctrl vq.
> > > 
> > > That will need a new feature bit.
> > > 
> > Yes. ctrl vq can tell what all configuration does it allow. :)
> > Or you prefer feature bit?
> 
> We did feature bits for this in the past.
> 
> > > > If merged buffer is done, and new mtu is > minimum posting size, no need
> > > to undergo vq reset.
> > > > If merged buffer is not done, and buffer posted are smaller than new 
> > > > mtu,
> > > undergo vq reset optionally.
> > > 
> > > This can be done with or without sending mtu to device.
> > Yes, telling mtu to device helps device to optimize and adhere to the spec 
> > line " The device MUST NOT pass received packets that exceed mtu" in 
> > section 5.1.4.1.
> 
> Again, that line refers to \field{mtu} which is the max mtu supported,
> irrespective to anything driver does.

BTW with any such ctrl vq interface we need to think how cases such as
increasing and decreasing MTU work.  The normal behaviour for linux
drivers is to limit this to when the link is down.  Which reminds me, we
do not have a command to bring link down, either.

> -- 
> MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

2022-08-10 Thread Michael S. Tsirkin
On Wed, Aug 10, 2022 at 04:22:41PM +, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin 
> > Sent: Wednesday, August 10, 2022 12:05 PM
> > 
> > On Wed, Aug 10, 2022 at 04:00:08PM +, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin 
> > > > Sent: Wednesday, August 10, 2022 5:03 AM
> > > > > >
> > > > > > Should we make this depend on the vq reset ability maybe?
> > > > >
> > > > > The advantage of this is to keep TX working. Or we can use device
> > > > > reset as a fallback if there's no vq reset.
> > > > >
> > > > > Thanks
> > > >
> > > > Device reset is really annoying in that it loses all the state:
> > > > rx filters etc etc.
> > >
> > > The elegant solution is let driver tell the new mtu to the device.
> > > One way to do so is by using existing ctrl vq.
> > 
> > That will need a new feature bit.
> > 
> Yes. ctrl vq can tell what all configuration does it allow. :)
> Or you prefer feature bit?

We did feature bits for this in the past.

> > > If merged buffer is done, and new mtu is > minimum posting size, no need
> > to undergo vq reset.
> > > If merged buffer is not done, and buffer posted are smaller than new mtu,
> > undergo vq reset optionally.
> > 
> > This can be done with or without sending mtu to device.
> Yes, telling mtu to device helps device to optimize and adhere to the spec 
> line " The device MUST NOT pass received packets that exceed mtu" in section 
> 5.1.4.1.

Again, that line refers to \field{mtu} which is the max mtu supported,
irrespective to anything driver does.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

2022-08-10 Thread Michael S. Tsirkin
On Wed, Aug 10, 2022 at 04:00:08PM +, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin 
> > Sent: Wednesday, August 10, 2022 5:03 AM
> > > >
> > > > Should we make this depend on the vq reset ability maybe?
> > >
> > > The advantage of this is to keep TX working. Or we can use device
> > > reset as a fallback if there's no vq reset.
> > >
> > > Thanks
> > 
> > Device reset is really annoying in that it loses all the state:
> > rx filters etc etc.
> 
> The elegant solution is let driver tell the new mtu to the device.
> One way to do so is by using existing ctrl vq.

That will need a new feature bit.

> If merged buffer is done, and new mtu is > minimum posting size, no need to 
> undergo vq reset.
> If merged buffer is not done, and buffer posted are smaller than new mtu, 
> undergo vq reset optionally.

This can be done with or without sending mtu to device.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



[virtio-dev] Re: [PATCH 1/4] content: Introduce driver/device auxiliary notifications

2022-08-10 Thread Cornelia Huck
On Wed, Aug 10 2022, "Michael S. Tsirkin"  wrote:

> On Wed, Aug 10, 2022 at 11:54:35AM +0200, Cornelia Huck wrote:
>> On Tue, Aug 09 2022, "Michael S. Tsirkin"  wrote:
>> 
>> > On Wed, Mar 30, 2022 at 04:21:02PM +0100, Usama Arif wrote:
>> >> Driver auxiliary notifications allow the device to send notifications
>> >> other than configuration changes and used buffer notifications to the
>> >> driver, these are optional and their meaning is device-specific.
>> >> 
>> >> Device auxiliary notifcations allow the driver to send notifcations
>> >> other than available buffer notifications to the device for example
>> >> through a device register, these are optional and their meaning is
>> >> device-specific.
>> >> 
>> >> These device-specific notifications are needed later when adding support
>> >> for virtio-vhost-user device.
>> >> 
>> >> Signed-off-by: Usama Arif 
>> >> Signed-off-by: Stefan Hajnoczi 
>> >> Signed-off-by: Nikos Dragazis 
>> >
>> > I see ccw is missing. Cornelia, any suggestions?
>> 
>> Hmm... I seem to be really behind on ccw things :(
>> 
>> We can probably use the following:
>> 
>> - for device->driver notification, use the next bit in the secondary
>>   indicators (bit 0 is used for config change notification)
>> - for driver->device notification, maybe use a new subcode for diagnose
>>   0x500 (4 is probably the next free one?)
>> 
>> I have not looked at the requirements deeply, though.
>> 
>> This highlights another problem, however: When we introduce new features
>> that require a transport-specific implementation, we often end up with a
>> PCI implementation, but sometimes MMIO and more often ccw are left
>> behind -- which is understandable, as PCI is what most people use, and
>> ccw is something only a very few people are familiar with. This sadly
>> means that we have a backlog of features supported in PCI, but not in
>> ccw... requiring implementations for ccw would put an undue burden on
>> contributors, as most of them are unlikely to write anything for a
>> mainframe, ever. On the flip side, I do not have enough bandwith to deal
>> with all of this.
>> 
>> Halil, any thoughts (on any of the above)?
>
> Kind of depends. We Do we want to add a "universal config"
> structure shared between transports?
> Will help with some use-cases though not this one.

A "universal config" structure seems like a good idea; but, as you said,
that will not help in all cases.

>
>
>> >
>> >> ---
>> >>  content.tex | 35 ++-
>> >>  1 file changed, 22 insertions(+), 13 deletions(-)
>> >> 
>> >> diff --git a/content.tex b/content.tex
>> >> index c6f116c..85980ac 100644
>> >> --- a/content.tex
>> >> +++ b/content.tex
>> >> @@ -160,29 +160,38 @@ \subsection{Legacy Interface: A Note on Feature
>> >>  Specification text within these sections generally does not apply
>> >>  to non-transitional devices.
>> >>  
>> >> -\section{Notifications}\label{sec:Basic Facilities of a Virtio Device
>> >> -/ Notifications}
>> >> +\section{Notifications}\label{sec:Basic Facilities of a Virtio Device / 
>> >> Notifications}
>> >>  
>> >>  The notion of sending a notification (driver to device or device
>> >>  to driver) plays an important role in this specification. The
>> >>  modus operandi of the notifications is transport specific.
>> >>  
>> >> -There are three types of notifications: 
>> >> +There are five types of notifications:
>> >>  \begin{itemize}
>> >>  \item configuration change notification
>> >>  \item available buffer notification
>> >> -\item used buffer notification. 
>> >> +\item used buffer notification
>> >> +\item driver auxiliary notification
>> >> +\item device auxiliary notification
>> >>  \end{itemize}
>> >>  
>> >> -Configuration change notifications and used buffer notifications are sent
>> >> -by the device, the recipient is the driver. A configuration change
>> >> -notification indicates that the device configuration space has changed; a
>> >> -used buffer notification indicates that a buffer may have been made used
>> >> -on the virtqueue designated by the notification.
>> >> -
>> >> -Available buffer notifications are sent by the driver, the recipient is
>> >> -the device. This type of notification indicates that a buffer may have
>> >> -been made available on the virtqueue designated by the notification.
>> >> +Configuration change notifications, used buffer notifications and
>> >> +driver auxiliary notifications are sent by the device,
>> >> +the recipient is the driver. A configuration change notification 
>> >> indicates
>> >> +that the device configuration space has changed; a used buffer 
>> >> notification
>> >> +indicates that a buffer may have been made used on the virtqueue 
>> >> designated
>> >> +by the notification; driver auxiliary notifications allow the
>> >> +device to send notifications other than configuration changes and used
>> >> +buffer notifications to the driver, these are optional and their meaning
>> >> +is device-specific.
>> >> +
>>

[virtio-dev] Re: [PATCH 1/4] content: Introduce driver/device auxiliary notifications

2022-08-10 Thread Michael S. Tsirkin
On Wed, Aug 10, 2022 at 11:54:35AM +0200, Cornelia Huck wrote:
> On Tue, Aug 09 2022, "Michael S. Tsirkin"  wrote:
> 
> > On Wed, Mar 30, 2022 at 04:21:02PM +0100, Usama Arif wrote:
> >> Driver auxiliary notifications allow the device to send notifications
> >> other than configuration changes and used buffer notifications to the
> >> driver, these are optional and their meaning is device-specific.
> >> 
> >> Device auxiliary notifcations allow the driver to send notifcations
> >> other than available buffer notifications to the device for example
> >> through a device register, these are optional and their meaning is
> >> device-specific.
> >> 
> >> These device-specific notifications are needed later when adding support
> >> for virtio-vhost-user device.
> >> 
> >> Signed-off-by: Usama Arif 
> >> Signed-off-by: Stefan Hajnoczi 
> >> Signed-off-by: Nikos Dragazis 
> >
> > I see ccw is missing. Cornelia, any suggestions?
> 
> Hmm... I seem to be really behind on ccw things :(
> 
> We can probably use the following:
> 
> - for device->driver notification, use the next bit in the secondary
>   indicators (bit 0 is used for config change notification)
> - for driver->device notification, maybe use a new subcode for diagnose
>   0x500 (4 is probably the next free one?)
> 
> I have not looked at the requirements deeply, though.
> 
> This highlights another problem, however: When we introduce new features
> that require a transport-specific implementation, we often end up with a
> PCI implementation, but sometimes MMIO and more often ccw are left
> behind -- which is understandable, as PCI is what most people use, and
> ccw is something only a very few people are familiar with. This sadly
> means that we have a backlog of features supported in PCI, but not in
> ccw... requiring implementations for ccw would put an undue burden on
> contributors, as most of them are unlikely to write anything for a
> mainframe, ever. On the flip side, I do not have enough bandwith to deal
> with all of this.
> 
> Halil, any thoughts (on any of the above)?

Kind of depends. We Do we want to add a "universal config"
structure shared between transports?
Will help with some use-cases though not this one.


> >
> >> ---
> >>  content.tex | 35 ++-
> >>  1 file changed, 22 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/content.tex b/content.tex
> >> index c6f116c..85980ac 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -160,29 +160,38 @@ \subsection{Legacy Interface: A Note on Feature
> >>  Specification text within these sections generally does not apply
> >>  to non-transitional devices.
> >>  
> >> -\section{Notifications}\label{sec:Basic Facilities of a Virtio Device
> >> -/ Notifications}
> >> +\section{Notifications}\label{sec:Basic Facilities of a Virtio Device / 
> >> Notifications}
> >>  
> >>  The notion of sending a notification (driver to device or device
> >>  to driver) plays an important role in this specification. The
> >>  modus operandi of the notifications is transport specific.
> >>  
> >> -There are three types of notifications: 
> >> +There are five types of notifications:
> >>  \begin{itemize}
> >>  \item configuration change notification
> >>  \item available buffer notification
> >> -\item used buffer notification. 
> >> +\item used buffer notification
> >> +\item driver auxiliary notification
> >> +\item device auxiliary notification
> >>  \end{itemize}
> >>  
> >> -Configuration change notifications and used buffer notifications are sent
> >> -by the device, the recipient is the driver. A configuration change
> >> -notification indicates that the device configuration space has changed; a
> >> -used buffer notification indicates that a buffer may have been made used
> >> -on the virtqueue designated by the notification.
> >> -
> >> -Available buffer notifications are sent by the driver, the recipient is
> >> -the device. This type of notification indicates that a buffer may have
> >> -been made available on the virtqueue designated by the notification.
> >> +Configuration change notifications, used buffer notifications and
> >> +driver auxiliary notifications are sent by the device,
> >> +the recipient is the driver. A configuration change notification indicates
> >> +that the device configuration space has changed; a used buffer 
> >> notification
> >> +indicates that a buffer may have been made used on the virtqueue 
> >> designated
> >> +by the notification; driver auxiliary notifications allow the
> >> +device to send notifications other than configuration changes and used
> >> +buffer notifications to the driver, these are optional and their meaning
> >> +is device-specific.
> >> +
> >> +Available buffer notifications and device auxiliary notifications
> >> +are sent by the driver, the recipient is the device. Available buffer
> >> +notifications indicate that a buffer may have been made available on the
> >> +virtqueue designated by the notification; de

[virtio-dev] [PATCH] virtio-mmio: Introduce virtio_mmio hotplug

2022-08-10 Thread Igor Skalkin
From: Igor Skalkin 

While the virtio device is not yet running, the virtual machine manager
advertises the device with device_id set to 0.
During virtio mmio probing, the device_id is checked, and if it is 0,
the rest of the probing function is deferred until the interrupt arrives.

Signed-off-by: Igor Skalkin 
---
In our setup, we have a Linux host running virtio devices and virtualised
Linux/Android Guest[s] running virtio drivers.
Situation "the guest OS calls the probe() function for the virtio driver,
but the virtio device has not yet started in the host OS." keeps happening.
Also, some devices need to be hot-plugged later instead of starting during
system sturtup.

Probing of the guest virtio drivers should be deferred until the host device
has started.
---
 drivers/virtio/virtio_mmio.c | 58 
 1 file changed, 58 insertions(+)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 083ff1eb743d..c2e28a8f 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -91,6 +91,8 @@ struct virtio_mmio_device {
/* a list of queues so we can dispatch IRQs */
spinlock_t lock;
struct list_head virtqueues;
+
+   struct work_struct hotplug_work;
 };
  struct virtio_mmio_vq_info {
@@ -592,6 +594,43 @@ static void virtio_mmio_release_dev(struct device *_d)
  /* Platform device */
 +static irqreturn_t hotplug_interrupt(int irq, void *opaque)
+{
+   struct virtio_mmio_device *vm_dev = opaque;
+
+   if (readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID))
+   schedule_work(&vm_dev->hotplug_work);
+
+   return IRQ_HANDLED;
+}
+
+static int virtio_mmio_request_irq(irq_handler_t handler,
+  struct virtio_mmio_device *vm_dev)
+{
+   int err;
+
+   err = request_irq(platform_get_irq(vm_dev->pdev, 0), handler,
+ IRQF_SHARED, dev_name(&vm_dev->pdev->dev), vm_dev);
+   if (err)
+   dev_err(&vm_dev->pdev->dev, "request_irq(%s) returns %d\n",
+   dev_name(&vm_dev->pdev->dev), err);
+
+   return err;
+}
+
+static int finish_probe(struct virtio_mmio_device *vm_dev);
+static void virtio_mmio_hotplug_work(struct work_struct *hotplug_work)
+{
+   struct virtio_mmio_device *vm_dev =
+   container_of(hotplug_work, struct virtio_mmio_device,
+   hotplug_work);
+
+   free_irq(platform_get_irq(vm_dev->pdev, 0), vm_dev);
+
+   if (finish_probe(vm_dev))
+   virtio_mmio_request_irq(hotplug_interrupt, vm_dev);
+}
+
 static int virtio_mmio_probe(struct platform_device *pdev)
 {
struct virtio_mmio_device *vm_dev;
@@ -628,6 +667,25 @@ static int virtio_mmio_probe(struct platform_device
*pdev)
return -ENXIO;
}
 +  vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
+   if (!vm_dev->vdev.id.device) {
+   rc = virtio_mmio_request_irq(hotplug_interrupt, vm_dev);
+   if (rc)
+   return rc;
+
+   INIT_WORK(&vm_dev->hotplug_work, virtio_mmio_hotplug_work);
+
+   return 0;
+   }
+
+   return finish_probe(vm_dev);
+}
+
+static int finish_probe(struct virtio_mmio_device *vm_dev)
+{
+   struct platform_device *pdev = vm_dev->pdev;
+   int rc;
+
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
if (vm_dev->vdev.id.device == 0) {
/*
--
2.37.1



Please mind our privacy 
notice
 pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 
DSGVO finden Sie 
hier.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



[virtio-dev] Re: [PATCH 1/4] content: Introduce driver/device auxiliary notifications

2022-08-10 Thread Cornelia Huck
On Tue, Aug 09 2022, "Michael S. Tsirkin"  wrote:

> On Wed, Mar 30, 2022 at 04:21:02PM +0100, Usama Arif wrote:
>> Driver auxiliary notifications allow the device to send notifications
>> other than configuration changes and used buffer notifications to the
>> driver, these are optional and their meaning is device-specific.
>> 
>> Device auxiliary notifcations allow the driver to send notifcations
>> other than available buffer notifications to the device for example
>> through a device register, these are optional and their meaning is
>> device-specific.
>> 
>> These device-specific notifications are needed later when adding support
>> for virtio-vhost-user device.
>> 
>> Signed-off-by: Usama Arif 
>> Signed-off-by: Stefan Hajnoczi 
>> Signed-off-by: Nikos Dragazis 
>
> I see ccw is missing. Cornelia, any suggestions?

Hmm... I seem to be really behind on ccw things :(

We can probably use the following:

- for device->driver notification, use the next bit in the secondary
  indicators (bit 0 is used for config change notification)
- for driver->device notification, maybe use a new subcode for diagnose
  0x500 (4 is probably the next free one?)

I have not looked at the requirements deeply, though.

This highlights another problem, however: When we introduce new features
that require a transport-specific implementation, we often end up with a
PCI implementation, but sometimes MMIO and more often ccw are left
behind -- which is understandable, as PCI is what most people use, and
ccw is something only a very few people are familiar with. This sadly
means that we have a backlog of features supported in PCI, but not in
ccw... requiring implementations for ccw would put an undue burden on
contributors, as most of them are unlikely to write anything for a
mainframe, ever. On the flip side, I do not have enough bandwith to deal
with all of this.

Halil, any thoughts (on any of the above)?

>
>> ---
>>  content.tex | 35 ++-
>>  1 file changed, 22 insertions(+), 13 deletions(-)
>> 
>> diff --git a/content.tex b/content.tex
>> index c6f116c..85980ac 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -160,29 +160,38 @@ \subsection{Legacy Interface: A Note on Feature
>>  Specification text within these sections generally does not apply
>>  to non-transitional devices.
>>  
>> -\section{Notifications}\label{sec:Basic Facilities of a Virtio Device
>> -/ Notifications}
>> +\section{Notifications}\label{sec:Basic Facilities of a Virtio Device / 
>> Notifications}
>>  
>>  The notion of sending a notification (driver to device or device
>>  to driver) plays an important role in this specification. The
>>  modus operandi of the notifications is transport specific.
>>  
>> -There are three types of notifications: 
>> +There are five types of notifications:
>>  \begin{itemize}
>>  \item configuration change notification
>>  \item available buffer notification
>> -\item used buffer notification. 
>> +\item used buffer notification
>> +\item driver auxiliary notification
>> +\item device auxiliary notification
>>  \end{itemize}
>>  
>> -Configuration change notifications and used buffer notifications are sent
>> -by the device, the recipient is the driver. A configuration change
>> -notification indicates that the device configuration space has changed; a
>> -used buffer notification indicates that a buffer may have been made used
>> -on the virtqueue designated by the notification.
>> -
>> -Available buffer notifications are sent by the driver, the recipient is
>> -the device. This type of notification indicates that a buffer may have
>> -been made available on the virtqueue designated by the notification.
>> +Configuration change notifications, used buffer notifications and
>> +driver auxiliary notifications are sent by the device,
>> +the recipient is the driver. A configuration change notification indicates
>> +that the device configuration space has changed; a used buffer notification
>> +indicates that a buffer may have been made used on the virtqueue designated
>> +by the notification; driver auxiliary notifications allow the
>> +device to send notifications other than configuration changes and used
>> +buffer notifications to the driver, these are optional and their meaning
>> +is device-specific.
>> +
>> +Available buffer notifications and device auxiliary notifications
>> +are sent by the driver, the recipient is the device. Available buffer
>> +notifications indicate that a buffer may have been made available on the
>> +virtqueue designated by the notification; device auxiliary
>> +notifcations allow the driver to send notifcations other than available
>> +buffer notifications to the device for example through a device register, 
>> these
>> +are optional and their meaning is device-specific.
>>  
>>  The semantics, the transport-specific implementations, and other
>>  important aspects of the different notifications are specified in detail
>> -- 
>> 2.25.1



Re: [virtio-dev] Re: [PATCH] virtio_net: support split header

2022-08-10 Thread Heng Qi


在 2022/8/9 下午5:18, Jason Wang 写道:

On Thu, Aug 4, 2022 at 8:48 PM Heng Qi  wrote:


在 2022/8/4 下午2:27, Jason Wang 写道:

On Mon, Aug 1, 2022 at 2:59 PM Heng Qi  wrote:

From: Xuan Zhuo

The purpose of this feature is to split the header and the payload of
the packet.

|receive buffer|
|   0th descriptor | 1th descriptor|
| virtnet hdr | mac | ip hdr | tcp hdr|<-- hold -->|   payload |

We can use a buffer plus a separate page when allocating the receive
buffer. In this way, we can ensure that all payloads can be
independently in a page, which is very beneficial for the zerocopy
implemented by the upper layer.

Signed-off-by: Xuan Zhuo
Signed-off-by: Heng Qi
Reviewed-by: Kangjie Xu
---

v6:
 1. Fix some syntax issues. @Cornelia Huck
 2. Clarify some paragraphs. @Cornelia Huck
 3. Determine the device what to do if it does not perform header split on 
a packet.

v5:
 1. Determine when hdr_len is credible in the process of rx
 2. Clean up the use of buffers and descriptors
 3. Clarify the meaning of used lenght if the first descriptor is skipped 
in the case of merge

v4:
 1. fix typo @Cornelia Huck @Jason Wang
 2. do not split header for IP fragmentation packet. @Jason Wang

v3:
 1. Fix some syntax issues
 2. Fix some terminology issues
 3. It is not unified with ip alignment, so ip alignment is not included
 4. Make it clear that the device must support four types, in the case of
 successful negotiation.

  conformance.tex |   2 +
  content.tex | 111 ++--
  2 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/conformance.tex b/conformance.tex
index 2b86fc6..bd0f463 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -150,6 +150,7 @@ \section{Conformance Targets}\label{sec:Conformance / 
Conformance Targets}
  \item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Offloads State Configuration / Setting Offloads State}
  \item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Receive-side scaling (RSS) }
  \item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Notifications Coalescing}
+\item \ref{drivernormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Split Header}
  \end{itemize}

  \conformance{\subsection}{Block Driver Conformance}\label{sec:Conformance / 
Driver Conformance / Block Driver Conformance}
@@ -415,6 +416,7 @@ \section{Conformance Targets}\label{sec:Conformance / 
Conformance Targets}
  \item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Automatic receive steering in multiqueue mode}
  \item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Receive-side scaling (RSS) / RSS processing}
  \item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Notifications Coalescing}
+\item \ref{devicenormative:Device Types / Network Device / Device Operation / 
Control Virtqueue / Split Header}
  \end{itemize}

  \conformance{\subsection}{Block Device Conformance}\label{sec:Conformance / 
Device Conformance / Block Device Conformance}
diff --git a/content.tex b/content.tex
index e863709..74c36fe 100644
--- a/content.tex
+++ b/content.tex
@@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
Network Device / Feature bits
  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
  channel.

+\item[VIRTIO_NET_F_SPLIT_HEADER (52)] Device supports splitting the protocol
+header and the payload.
+
  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.

  \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
@@ -3140,6 +3143,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device 
Types / Network Device
  \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
  \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or 
VIRTIO_NET_F_HOST_TSO6.
  \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
+\item[VIRTIO_NET_F_SPLIT_HEADER] Requires VIRTIO_NET_F_CTRL_VQ.
  \end{description}

  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / 
Network Device / Feature bits / Legacy Interface: Feature bits}
@@ -3371,6 +3375,7 @@ \subsection{Device Operation}\label{sec:Device Types / 
Network Device / Device O
  #define VIRTIO_NET_HDR_F_NEEDS_CSUM1
  #define VIRTIO_NET_HDR_F_DATA_VALID2
  #define VIRTIO_NET_HDR_F_RSC_INFO  4
+#define VIRTIO_NET_HDR_F_SPLIT_HEADER  8
  u8 flags;
  #define VIRTIO_NET_HDR_GSO_NONE0
  #define VIRTIO_NET_HDR_GSO_TCPV4   1
@@ -3799,9 +3804,10 @@ \subsubsection{Processing of Incoming 
Packets}\label{sec:Device Types / Network
  not set VIRTIO

Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

2022-08-10 Thread Michael S. Tsirkin
On Wed, Aug 10, 2022 at 02:59:55PM +0800, Jason Wang wrote:
> On Wed, Aug 10, 2022 at 2:15 PM Michael S. Tsirkin  wrote:
> >
> > On Wed, Aug 10, 2022 at 02:14:07AM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Aug 09, 2022 at 04:24:23PM -0700, Si-Wei Liu wrote:
> > > >
> > > >
> > > > On 8/9/2022 3:49 PM, Parav Pandit wrote:
> > > > > > From: Michael S. Tsirkin 
> > > > > > Sent: Tuesday, August 9, 2022 6:26 PM
> > > > > > To: Parav Pandit 
> > > > > > Cc: Si-Wei Liu ; Jason Wang
> > > > > > ; Gavin Li ; Hemminger,
> > > > > > Stephen ; davem
> > > > > > ; virtualization  > > > > > foundation.org>; Virtio-Dev ;
> > > > > > jesse.brandeb...@intel.com; alexander.h.du...@intel.com;
> > > > > > kubak...@wp.pl; sridhar.samudr...@intel.com; losewe...@gmail.com; 
> > > > > > Gavi
> > > > > > Teitz 
> > > > > > Subject: Re: [virtio-dev] [PATCH] virtio-net: use mtu size as 
> > > > > > buffer length for
> > > > > > big packets
> > > > > >
> > > > > > On Tue, Aug 09, 2022 at 09:49:03PM +, Parav Pandit wrote:
> > > > > > > > From: Michael S. Tsirkin 
> > > > > > > > Sent: Tuesday, August 9, 2022 5:38 PM
> > > > > > > [..]
> > > > > > > > > > I think virtio-net driver doesn't differentiate MTU and 
> > > > > > > > > > MRU, in
> > > > > > > > > > which case the receive buffer will be reduced to fit the 
> > > > > > > > > > 1500B
> > > > > > > > > > payload size when mtu is lowered down to 1500 from 9000.
> > > > > > > > > How? Driver reduced the mXu to 1500, say it is improved to 
> > > > > > > > > post
> > > > > > > > > buffers of
> > > > > > > > 1500 bytes.
> > > > > > > > > Device doesn't know about it because mtu in config space is 
> > > > > > > > > RO field.
> > > > > > > > > Device keep dropping 9K packets because buffers posted are 
> > > > > > > > > 1500
> > > > > > bytes.
> > > > > > > > > This is because device follows the spec " The device MUST NOT 
> > > > > > > > > pass
> > > > > > > > received packets that exceed mtu".
> > > > > > > >
> > > > > > > >
> > > > > > > > The "mtu" here is the device config field, which is
> > > > > > > >
> > > > > > > >  /* Default maximum transmit unit advice */
> > > > > > > >
> > > > > > > It is the field from struct virtio_net_config.mtu. right?
> > > > > > > This is RO field for driver.
> > > > > > >
> > > > > > > > there is no guarantee device will not get a bigger packet.
> > > > > > > Right. That is what I also hinted.
> > > > > > > Hence, allocating buffers worth upto mtu is safer.
> > > > > > yes
> > > > > >
> > > > > > > When user overrides it, driver can be further optimized to honor 
> > > > > > > such new
> > > > > > value on rx buffer posting.
> > > > > >
> > > > > > no, not without a feature bit promising device won't get wedged.
> > > > > >
> > > > > I mean to say as_it_stands today, driver can decide to post smaller 
> > > > > buffers with larger mtu.
> > > > > Why device should be affected with it?
> > > > > ( I am not proposing such weird configuration but asking for sake of 
> > > > > correctness).
> > > > I am also confused how the device can be wedged in this case.
> > >
> > > Yea sorry. I misunderstood the code. It can't be.
> >
> > Here's a problem as I see it. Let's say we reduce mtu.
> > Small buffers are added. Now we increase mtu.
> > Device will drop all packets until small buffers are consumed.
> >
> > Should we make this depend on the vq reset ability maybe?
> 
> The advantage of this is to keep TX working. Or we can use device
> reset as a fallback if there's no vq reset.
> 
> Thanks

Device reset is really annoying in that it loses all the state:
rx filters etc etc.

> 
> >
> > > --
> > > MST
> >


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

2022-08-10 Thread Jason Wang
On Wed, Aug 10, 2022 at 2:15 PM Michael S. Tsirkin  wrote:
>
> On Wed, Aug 10, 2022 at 02:14:07AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Aug 09, 2022 at 04:24:23PM -0700, Si-Wei Liu wrote:
> > >
> > >
> > > On 8/9/2022 3:49 PM, Parav Pandit wrote:
> > > > > From: Michael S. Tsirkin 
> > > > > Sent: Tuesday, August 9, 2022 6:26 PM
> > > > > To: Parav Pandit 
> > > > > Cc: Si-Wei Liu ; Jason Wang
> > > > > ; Gavin Li ; Hemminger,
> > > > > Stephen ; davem
> > > > > ; virtualization  > > > > foundation.org>; Virtio-Dev ;
> > > > > jesse.brandeb...@intel.com; alexander.h.du...@intel.com;
> > > > > kubak...@wp.pl; sridhar.samudr...@intel.com; losewe...@gmail.com; Gavi
> > > > > Teitz 
> > > > > Subject: Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer 
> > > > > length for
> > > > > big packets
> > > > >
> > > > > On Tue, Aug 09, 2022 at 09:49:03PM +, Parav Pandit wrote:
> > > > > > > From: Michael S. Tsirkin 
> > > > > > > Sent: Tuesday, August 9, 2022 5:38 PM
> > > > > > [..]
> > > > > > > > > I think virtio-net driver doesn't differentiate MTU and MRU, 
> > > > > > > > > in
> > > > > > > > > which case the receive buffer will be reduced to fit the 1500B
> > > > > > > > > payload size when mtu is lowered down to 1500 from 9000.
> > > > > > > > How? Driver reduced the mXu to 1500, say it is improved to post
> > > > > > > > buffers of
> > > > > > > 1500 bytes.
> > > > > > > > Device doesn't know about it because mtu in config space is RO 
> > > > > > > > field.
> > > > > > > > Device keep dropping 9K packets because buffers posted are 1500
> > > > > bytes.
> > > > > > > > This is because device follows the spec " The device MUST NOT 
> > > > > > > > pass
> > > > > > > received packets that exceed mtu".
> > > > > > >
> > > > > > >
> > > > > > > The "mtu" here is the device config field, which is
> > > > > > >
> > > > > > >  /* Default maximum transmit unit advice */
> > > > > > >
> > > > > > It is the field from struct virtio_net_config.mtu. right?
> > > > > > This is RO field for driver.
> > > > > >
> > > > > > > there is no guarantee device will not get a bigger packet.
> > > > > > Right. That is what I also hinted.
> > > > > > Hence, allocating buffers worth upto mtu is safer.
> > > > > yes
> > > > >
> > > > > > When user overrides it, driver can be further optimized to honor 
> > > > > > such new
> > > > > value on rx buffer posting.
> > > > >
> > > > > no, not without a feature bit promising device won't get wedged.
> > > > >
> > > > I mean to say as_it_stands today, driver can decide to post smaller 
> > > > buffers with larger mtu.
> > > > Why device should be affected with it?
> > > > ( I am not proposing such weird configuration but asking for sake of 
> > > > correctness).
> > > I am also confused how the device can be wedged in this case.
> >
> > Yea sorry. I misunderstood the code. It can't be.
>
> Here's a problem as I see it. Let's say we reduce mtu.
> Small buffers are added. Now we increase mtu.
> Device will drop all packets until small buffers are consumed.
>
> Should we make this depend on the vq reset ability maybe?

The advantage of this is to keep TX working. Or we can use device
reset as a fallback if there's no vq reset.

Thanks


>
> > --
> > MST
>


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org