On Sun, Oct 01, 2017 at 04:08:29AM +0000, Michael S. Tsirkin wrote:>On Thu, Sep 28, 2017 at 09:44:35AM +0000, Liang, Cunming wrote: >> >> Get it now. Please correct me if I missing something. >> >> >> Flags status hints, >> >> - DESC_DRIVER only: driver owns the descriptor w/o available info ready for device to use >> >> - DESC_DRIVER | DESC_WRAP: driver has prepared an available descriptor, device hasn't used it yet >> >> - None: device has used the descriptor, and write descriptor out >> >> - DESC_WRAP only: shall not happen, device make sure to clear it >> >> >> Polling behavior is, >> >> - Device monitor DESC_WRAP bit set or not; If set, go to use descriptor and clear DESC_DRIVER bit in the end (note: always need to clear DESC_WRAP) >> >> - Driver monitor DESC_DRIVER bit cleared or not; If cleared, reclaim descriptor(set DESC_DRIVER) and set DESC_WRAP once new available descriptor get ready to go >> >> >> -- >> Steve > > >Hmm no, not what I had in mind. > >DESC_DRIVER: used by driver to poll. Driver sets it when writing a >descriptor. Device clears it when overwriting a descriptor. >Thus driver uses DESC_DRIVER to detect that device data in >descriptor is valid.Basically DESC_HW from v2 split in two?> >DESC_WRAP: used by device to poll. Driver sets it to a *different* >value every time it overwrites a descriptor. >How to achieve it? >since descriptors are written out in ring order, >simply maintain the current value internally (start value 1) and flip it >every time you overwrite the first descriptor. >Device leaves it intact when overwriting a descriptor.This is confusing me a bit. My understanding is: 1. the internally kept wrap value only flipped when the first descriptor is overwritten 2. the moment the first descriptor is written the internal wrap value is flipped 0->1 or 1->0 and this value is written to every descriptor DESC_WRAP until we reach the first descriptor again> >After writing down this explanation, I think the names aren't >great. > >Let me try an alternative explanation. > >--------------- >A two-bit field, DRIVER_OWNER, signals the buffer ownership. >It has 4 possible values: >values 0x1, 0x11 are written by driver >values 0x0, 0x10 are written by deviceThe 0x prefix might add to the confusion here. It is really just two bits, no?>each time driver writes out a descriptor, it must make sure >that the high bit in OWNER changes. > >each time device writes out a descriptor, it must make sure >that the high bit in OWNER does not change. > >this is exactly the same functionally, DRIVER is high bit and >WRAP is the low bit. Does this make things clearer?So far it makes sense to me.>--------------- > > > >Maybe the difference between device and driver >is confusing. We can fix that by changing the values. >Here is an alternative. Let me know if you like it better - >I need to think a bit more to make sure it works, >but to give you an idea: > > >--------------- >A two-bit field, DRIVER_OWNER, signals the buffer ownership. >It has 4 possible values: >values 0x1, 0x10 are written by driver >values 0x0, 0x11 are written by device > >each time driver writes out a descriptor, it must make sure >that the high bit in OWNER changes. Thus first time >it writes 0x10, next time 0x1, then 0x10 again. > >each time device writes out a descriptor, it must make sure >that the low bit in OWNER changes. Thus first time >it writes 0x11, next time 0x0, then 0x11 again.DESC_WRAP is changed by the device now, so this would work differently than in the scenario from above. This would mean we don't need the internally kept wrap value, right? regards, Jens
On Wed, Oct 04, 2017 at 02:39:01PM +0200, Jens Freimann wrote:> On Sun, Oct 01, 2017 at 04:08:29AM +0000, Michael S. Tsirkin wrote: > > On Thu, Sep 28, 2017 at 09:44:35AM +0000, Liang, Cunming wrote: > > > > > > Get it now. Please correct me if I missing something. > > > > > > > > > Flags status hints, > > > > > > - DESC_DRIVER only: driver owns the descriptor w/o available info ready for device to use > > > > > > - DESC_DRIVER | DESC_WRAP: driver has prepared an available descriptor, device hasn't used it yet > > > > > > - None: device has used the descriptor, and write descriptor out > > > > > > - DESC_WRAP only: shall not happen, device make sure to clear it > > > > > > > > > Polling behavior is, > > > > > > - Device monitor DESC_WRAP bit set or not; If set, go to use descriptor and clear DESC_DRIVER bit in the end (note: always need to clear DESC_WRAP) > > > > > > - Driver monitor DESC_DRIVER bit cleared or not; If cleared, reclaim descriptor(set DESC_DRIVER) and set DESC_WRAP once new available descriptor get ready to go > > > > > > > > > -- > > > Steve > > > > > > Hmm no, not what I had in mind. > > > > DESC_DRIVER: used by driver to poll. Driver sets it when writing a > > descriptor. Device clears it when overwriting a descriptor. > > Thus driver uses DESC_DRIVER to detect that device data in > > descriptor is valid. > > Basically DESC_HW from v2 split in two?Yes in order to avoid overwriting all descriptors.> > > > DESC_WRAP: used by device to poll. Driver sets it to a *different* > > value every time it overwrites a descriptor. How to achieve it? > > since descriptors are written out in ring order, > > simply maintain the current value internally (start value 1) and flip it > > every time you overwrite the first descriptor. > > Device leaves it intact when overwriting a descriptor. > > This is confusing me a bit. > > My understanding is: 1. the internally kept wrap value only flipped when the > first > descriptor is overwritten > > 2. the moment the first descriptor is written the internal wrap value > is flipped 0->1 or 1->0 and this value is written to every descriptor > DESC_WRAP until we reach the first descriptor againYes this is what I tried to say. Can you suggest a better wording then?> > > > After writing down this explanation, I think the names aren't > > great. > > > > Let me try an alternative explanation. > > > > --------------- > > A two-bit field, DRIVER_OWNER, signals the buffer ownership. > > It has 4 possible values: > > values 0x1, 0x11 are written by driver > > values 0x0, 0x10 are written by device > > The 0x prefix might add to the confusion here. It is really just two > bits, no?Ouch. Yes I meant 0b. Thanks!> > each time driver writes out a descriptor, it must make sure > > that the high bit in OWNER changes. > > > > each time device writes out a descriptor, it must make sure > > that the high bit in OWNER does not change. > > > > this is exactly the same functionally, DRIVER is high bit and > > WRAP is the low bit. Does this make things clearer? > > So far it makes sense to me. > > --------------- > > > > > > > > Maybe the difference between device and driver > > is confusing. We can fix that by changing the values. > > Here is an alternative. Let me know if you like it better - > > I need to think a bit more to make sure it works, > > but to give you an idea: > > > > > > --------------- > > A two-bit field, DRIVER_OWNER, signals the buffer ownership. > > It has 4 possible values: > > values 0x1, 0x10 are written by driver > > values 0x0, 0x11 are written by device > > > > each time driver writes out a descriptor, it must make sure > > that the high bit in OWNER changes. Thus first time > > it writes 0x10, next time 0x1, then 0x10 again. > > > > each time device writes out a descriptor, it must make sure > > that the low bit in OWNER changes. Thus first time > > it writes 0x11, next time 0x0, then 0x11 again. > > DESC_WRAP is changed by the device now, so this would work differently > than in the scenario from above. This would mean we don't need the > internally kept wrap value, right?I think no but I did not complete simulating this yet so I don't yet know if it even works.> > regards, > Jens
> -----Original Message----- > From: virtio-dev at lists.oasis-open.org [mailto:virtio-dev at lists.oasis-open.org] > On Behalf Of Michael S. Tsirkin > Sent: Wednesday, October 4, 2017 8:58 PM > To: Jens Freimann <jfreimann at redhat.com> > Cc: Liang, Cunming <cunming.liang at intel.com>; Steven Luong (sluong) > <sluong at cisco.com>; virtio-dev at lists.oasis-open.org; > virtualization at lists.linux-foundation.org > Subject: Re: [virtio-dev] packed ring layout proposal v3 > > On Wed, Oct 04, 2017 at 02:39:01PM +0200, Jens Freimann wrote: > > On Sun, Oct 01, 2017 at 04:08:29AM +0000, Michael S. Tsirkin wrote: > > > On Thu, Sep 28, 2017 at 09:44:35AM +0000, Liang, Cunming wrote: > > > > > > > > Get it now. Please correct me if I missing something. > > > > > > > > > > > > Flags status hints, > > > > > > > > - DESC_DRIVER only: driver owns the descriptor w/o available info > > > > ready for device to use > > > > > > > > - DESC_DRIVER | DESC_WRAP: driver has prepared an available > > > > descriptor, device hasn't used it yet > > > > > > > > - None: device has used the descriptor, and write descriptor out > > > > > > > > - DESC_WRAP only: shall not happen, device make sure to clear it > > > > > > > > > > > > Polling behavior is, > > > > > > > > - Device monitor DESC_WRAP bit set or not; If set, go to use > > > > descriptor and clear DESC_DRIVER bit in the end (note: always need > > > > to clear DESC_WRAP) > > > > > > > > - Driver monitor DESC_DRIVER bit cleared or not; If cleared, > > > > reclaim descriptor(set DESC_DRIVER) and set DESC_WRAP once new > > > > available descriptor get ready to go > > > > > > > > > > > > -- > > > > Steve > > > > > > > > > Hmm no, not what I had in mind. > > > > > > DESC_DRIVER: used by driver to poll. Driver sets it when writing a > > > descriptor. Device clears it when overwriting a descriptor. > > > Thus driver uses DESC_DRIVER to detect that device data in > > > descriptor is valid. > > > > Basically DESC_HW from v2 split in two? > > Yes in order to avoid overwriting all descriptors. > > > > > > > DESC_WRAP: used by device to poll. Driver sets it to a *different* > > > value every time it overwrites a descriptor. How to achieve it? > > > since descriptors are written out in ring order, simply maintain the > > > current value internally (start value 1) and flip it every time you > > > overwrite the first descriptor. > > > Device leaves it intact when overwriting a descriptor.Ok, get it now.> > > > This is confusing me a bit. > > > > My understanding is: 1. the internally kept wrap value only flipped > > when the first descriptor is overwritten > > > > 2. the moment the first descriptor is written the internal wrap value > > is flipped 0->1 or 1->0 and this value is written to every descriptor > > DESC_WRAP until we reach the first descriptor againThat's right, it's also my take. DESC_WRAP is only used by driver, device does nothing with that flag.> > Yes this is what I tried to say. Can you suggest a better wording then?The term of DESC_WRAP is fine to me.> > > > > > > After writing down this explanation, I think the names aren't great. > > > > > > Let me try an alternative explanation. > > > > > > --------------- > > > A two-bit field, DRIVER_OWNER, signals the buffer ownership. > > > It has 4 possible values: > > > values 0x1, 0x11 are written by driver values 0x0, 0x10 are written > > > by device > > > > The 0x prefix might add to the confusion here. It is really just two > > bits, no? > > Ouch. Yes I meant 0b. Thanks!0b00, 0b10 are written by device? I suppose device can only clear high bit, can keep low bit no change. Then the value written by device can be either 0b01 or 0b00, but 0b10 means to set high bit, no?> > > > each time driver writes out a descriptor, it must make sure that the > > > high bit in OWNER changes. > > > > > > each time device writes out a descriptor, it must make sure that the > > > high bit in OWNER does not change.Typo here? It should be "..., it must make sure that the low bit in OWNER does not change."? For high bit in OWNER, each time devices writes out a descriptor, it must make sure to clear high bit in OWNER.> > > > > > this is exactly the same functionally, DRIVER is high bit and WRAP > > > is the low bit. Does this make things clearer? > > > > So far it makes sense to me.It sounds good.> > > --------------- > > > > > > > > > > > > Maybe the difference between device and driver is confusing. We can > > > fix that by changing the values. > > > Here is an alternative. Let me know if you like it better - I need > > > to think a bit more to make sure it works, but to give you an idea: > > > > > > > > > --------------- > > > A two-bit field, DRIVER_OWNER, signals the buffer ownership. > > > It has 4 possible values: > > > values 0x1, 0x10 are written by driver values 0x0, 0x11 are written > > > by device > > > > > > each time driver writes out a descriptor, it must make sure that the > > > high bit in OWNER changes. Thus first time it writes 0x10, next time > > > 0x1, then 0x10 again. > > > > > > each time device writes out a descriptor, it must make sure that the > > > low bit in OWNER changes. Thus first time it writes 0x11, next time > > > 0x0, then 0x11 again. > > > > DESC_WRAP is changed by the device now, so this would work differently > > than in the scenario from above. This would mean we don't need the > > internally kept wrap value, right? > > I think no but I did not complete simulating this yet so I don't yet know if it > even works. > > > > > regards, > > Jens > > --------------------------------------------------------------------- > To unsubscribe, e-mail: virtio-dev-unsubscribe at lists.oasis-open.org > For additional commands, e-mail: virtio-dev-help at lists.oasis-open.org