diff mbox series

examples/vhost: fix ioat ring space in callbacks

Message ID 20210317054054.34616-1-Cheng1.jiang@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series examples/vhost: fix ioat ring space in callbacks |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/travis-robot success travis build: passed
ci/github-robot success github build: passed
ci/iol-abi-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-testing warning Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Jiang, Cheng1 March 17, 2021, 5:40 a.m. UTC 
  We use ioat ring space for determining if ioat callbacks can enqueue a
packet to ioat device. But there is one slot can't be used in ioat
ring due to the ioat driver design, so we need to reduce one slot in
ioat ring to prevent ring size mismatch in ioat callbacks.

Fixes: 2aa47e94bfb2 ("examples/vhost: add ioat ring space count and check")
Cc: stable@dpdk.org

Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
---
 examples/vhost/ioat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Hu, Jiayu March 17, 2021, 6:58 a.m. UTC | #1 
Reviewed-by: Jiayu Hu <jiayu.hu@intel.com>

> -----Original Message-----
> From: Jiang, Cheng1 <cheng1.jiang@intel.com>
> Sent: Wednesday, March 17, 2021 1:41 PM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>; Wang, Yinan <yinan.wang@intel.com>; Jiang,
> Cheng1 <cheng1.jiang@intel.com>; stable@dpdk.org
> Subject: [PATCH] examples/vhost: fix ioat ring space in callbacks
> 
> We use ioat ring space for determining if ioat callbacks can enqueue a
> packet to ioat device. But there is one slot can't be used in ioat
> ring due to the ioat driver design, so we need to reduce one slot in
> ioat ring to prevent ring size mismatch in ioat callbacks.
> 
> Fixes: 2aa47e94bfb2 ("examples/vhost: add ioat ring space count and check")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
> ---
>  examples/vhost/ioat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c
> index 60b73be93..9cb5e0d50 100644
> --- a/examples/vhost/ioat.c
> +++ b/examples/vhost/ioat.c
> @@ -113,7 +113,7 @@ open_ioat(const char *value)
>  			goto out;
>  		}
>  		rte_rawdev_start(dev_id);
> -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
> +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
>  		dma_info->nr++;
>  		i++;
>  	}
> --
> 2.29.2
Maxime Coquelin April 7, 2021, 7:47 a.m. UTC | #2 
On 3/17/21 6:40 AM, Cheng Jiang wrote:
> We use ioat ring space for determining if ioat callbacks can enqueue a
> packet to ioat device. But there is one slot can't be used in ioat
> ring due to the ioat driver design, so we need to reduce one slot in
> ioat ring to prevent ring size mismatch in ioat callbacks.
> 
> Fixes: 2aa47e94bfb2 ("examples/vhost: add ioat ring space count and check")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
> ---
>  examples/vhost/ioat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c
> index 60b73be93..9cb5e0d50 100644
> --- a/examples/vhost/ioat.c
> +++ b/examples/vhost/ioat.c
> @@ -113,7 +113,7 @@ open_ioat(const char *value)
>  			goto out;
>  		}
>  		rte_rawdev_start(dev_id);
> -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
> +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;

That really comforts me in thinking we need a generic abstraction for
DMA devices. How is the application developer supposed to know that
the DMA driver has such weird limitations?

Can the driver be fixed to have a proper behavior?

>  		dma_info->nr++;
>  		i++;
>  	}
>
Hu, Jiayu April 7, 2021, 7:54 a.m. UTC | #3 
Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, April 7, 2021 3:48 PM
> To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>; Wang, Yinan <yinan.wang@intel.com>;
> stable@dpdk.org
> Subject: Re: [PATCH] examples/vhost: fix ioat ring space in callbacks
> 
> 
> 
> On 3/17/21 6:40 AM, Cheng Jiang wrote:
> > We use ioat ring space for determining if ioat callbacks can enqueue a
> > packet to ioat device. But there is one slot can't be used in ioat
> > ring due to the ioat driver design, so we need to reduce one slot in
> > ioat ring to prevent ring size mismatch in ioat callbacks.
> >
> > Fixes: 2aa47e94bfb2 ("examples/vhost: add ioat ring space count and
> check")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
> > ---
> >  examples/vhost/ioat.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c
> > index 60b73be93..9cb5e0d50 100644
> > --- a/examples/vhost/ioat.c
> > +++ b/examples/vhost/ioat.c
> > @@ -113,7 +113,7 @@ open_ioat(const char *value)
> >  			goto out;
> >  		}
> >  		rte_rawdev_start(dev_id);
> > -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
> > +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
> 
> That really comforts me in thinking we need a generic abstraction for
> DMA devices. How is the application developer supposed to know that
> the DMA driver has such weird limitations?
> 
> Can the driver be fixed to have a proper behavior?

Here is the patch of providing capacity check API for DMA:
http://patches.dpdk.org/project/dpdk/patch/20210318182042.43658-6-bruce.richardson@intel.com/

Thanks,
Jiayu
> 
> >  		dma_info->nr++;
> >  		i++;
> >  	}
> >
Thomas Monjalon April 7, 2021, 8:26 a.m. UTC | #4 
07/04/2021 09:47, Maxime Coquelin:
> 
> On 3/17/21 6:40 AM, Cheng Jiang wrote:
> > We use ioat ring space for determining if ioat callbacks can enqueue a
> > packet to ioat device. But there is one slot can't be used in ioat
> > ring due to the ioat driver design, so we need to reduce one slot in
> > ioat ring to prevent ring size mismatch in ioat callbacks.
> > 
> > Fixes: 2aa47e94bfb2 ("examples/vhost: add ioat ring space count and check")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
> > ---
> >  examples/vhost/ioat.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c
> > index 60b73be93..9cb5e0d50 100644
> > --- a/examples/vhost/ioat.c
> > +++ b/examples/vhost/ioat.c
> > @@ -113,7 +113,7 @@ open_ioat(const char *value)
> >  			goto out;
> >  		}
> >  		rte_rawdev_start(dev_id);
> > -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
> > +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
> 
> That really comforts me in thinking we need a generic abstraction for
> DMA devices. How is the application developer supposed to know that
> the DMA driver has such weird limitations?

Having a generic DMA API may be interesting.
Do you know any other HW candidate for such an API?
Do you think rte_memcpy can be used as a SW driver?
Thomas Monjalon April 7, 2021, 8:43 a.m. UTC | #5 
+Cc more people to start a discussion about a potential DMA API.
If you think it is interesting, we can start a fresh discussion thread.

07/04/2021 10:26, Thomas Monjalon:
> 07/04/2021 09:47, Maxime Coquelin:
> > 
> > On 3/17/21 6:40 AM, Cheng Jiang wrote:
> > > We use ioat ring space for determining if ioat callbacks can enqueue a
> > > packet to ioat device. But there is one slot can't be used in ioat
> > > ring due to the ioat driver design, so we need to reduce one slot in
> > > ioat ring to prevent ring size mismatch in ioat callbacks.
[...]
> > > --- a/examples/vhost/ioat.c
> > > +++ b/examples/vhost/ioat.c
> > > @@ -113,7 +113,7 @@ open_ioat(const char *value)
> > >  			goto out;
> > >  		}
> > >  		rte_rawdev_start(dev_id);
> > > -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
> > > +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
> > 
> > That really comforts me in thinking we need a generic abstraction for
> > DMA devices. How is the application developer supposed to know that
> > the DMA driver has such weird limitations?
> 
> Having a generic DMA API may be interesting.
> Do you know any other HW candidate for such an API?
> Do you think rte_memcpy can be used as a SW driver?
Maxime Coquelin April 7, 2021, 8:48 a.m. UTC | #6 
On 4/7/21 10:26 AM, Thomas Monjalon wrote:
> 07/04/2021 09:47, Maxime Coquelin:
>>
>> On 3/17/21 6:40 AM, Cheng Jiang wrote:
>>> We use ioat ring space for determining if ioat callbacks can enqueue a
>>> packet to ioat device. But there is one slot can't be used in ioat
>>> ring due to the ioat driver design, so we need to reduce one slot in
>>> ioat ring to prevent ring size mismatch in ioat callbacks.
>>>
>>> Fixes: 2aa47e94bfb2 ("examples/vhost: add ioat ring space count and check")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
>>> ---
>>>  examples/vhost/ioat.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c
>>> index 60b73be93..9cb5e0d50 100644
>>> --- a/examples/vhost/ioat.c
>>> +++ b/examples/vhost/ioat.c
>>> @@ -113,7 +113,7 @@ open_ioat(const char *value)
>>>  			goto out;
>>>  		}
>>>  		rte_rawdev_start(dev_id);
>>> -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
>>> +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
>>
>> That really comforts me in thinking we need a generic abstraction for
>> DMA devices. How is the application developer supposed to know that
>> the DMA driver has such weird limitations?
> 
> Having a generic DMA API may be interesting.
> Do you know any other HW candidate for such an API?
> Do you think rte_memcpy can be used as a SW driver?

Yes, I guess we could create a vdev driver with MEM_TO_MEM capability
using rte_memcpy().

Note that IOAT in the Kernel is supported by the DMA framework.

Regards,
Maxime
Maxime Coquelin April 13, 2021, 8:50 a.m. UTC | #7 
On 4/7/21 9:54 AM, Hu, Jiayu wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, April 7, 2021 3:48 PM
>> To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo
>> <chenbo.xia@intel.com>; Thomas Monjalon <thomas@monjalon.net>
>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Yang, YvonneX
>> <yvonnex.yang@intel.com>; Wang, Yinan <yinan.wang@intel.com>;
>> stable@dpdk.org
>> Subject: Re: [PATCH] examples/vhost: fix ioat ring space in callbacks
>>
>>
>>
>> On 3/17/21 6:40 AM, Cheng Jiang wrote:
>>> We use ioat ring space for determining if ioat callbacks can enqueue a
>>> packet to ioat device. But there is one slot can't be used in ioat
>>> ring due to the ioat driver design, so we need to reduce one slot in
>>> ioat ring to prevent ring size mismatch in ioat callbacks.
>>>
>>> Fixes: 2aa47e94bfb2 ("examples/vhost: add ioat ring space count and
>> check")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
>>> ---
>>>  examples/vhost/ioat.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c
>>> index 60b73be93..9cb5e0d50 100644
>>> --- a/examples/vhost/ioat.c
>>> +++ b/examples/vhost/ioat.c
>>> @@ -113,7 +113,7 @@ open_ioat(const char *value)
>>>  			goto out;
>>>  		}
>>>  		rte_rawdev_start(dev_id);
>>> -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
>>> +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
>>
>> That really comforts me in thinking we need a generic abstraction for
>> DMA devices. How is the application developer supposed to know that
>> the DMA driver has such weird limitations?
>>
>> Can the driver be fixed to have a proper behavior?
> 
> Here is the patch of providing capacity check API for DMA:
> http://patches.dpdk.org/project/dpdk/patch/20210318182042.43658-6-bruce.richardson@intel.com/

OK, thanks for the pointer.

While this new API is being reviewed, and for LTS, let's pick your
patch. As soon as Bruce patch is merged, please send a new patch on top
to make use of this API.

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

> Thanks,
> Jiayu
>>
>>>  		dma_info->nr++;
>>>  		i++;
>>>  	}
>>>
>
Jiang, Cheng1 April 13, 2021, 9:55 a.m. UTC | #8 
Hi,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, April 13, 2021 4:51 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; Jiang, Cheng1 <cheng1.jiang@intel.com>;
> Xia, Chenbo <chenbo.xia@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: dev@dpdk.org; Yang, YvonneX <yvonnex.yang@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH] examples/vhost: fix ioat ring space in callbacks
> 
> 
> 
> On 4/7/21 9:54 AM, Hu, Jiayu wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Wednesday, April 7, 2021 3:48 PM
> >> To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo
> >> <chenbo.xia@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Yang, YvonneX
> >> <yvonnex.yang@intel.com>; Wang, Yinan <yinan.wang@intel.com>;
> >> stable@dpdk.org
> >> Subject: Re: [PATCH] examples/vhost: fix ioat ring space in callbacks
> >>
> >>
> >>
> >> On 3/17/21 6:40 AM, Cheng Jiang wrote:
> >>> We use ioat ring space for determining if ioat callbacks can enqueue
> >>> a packet to ioat device. But there is one slot can't be used in ioat
> >>> ring due to the ioat driver design, so we need to reduce one slot in
> >>> ioat ring to prevent ring size mismatch in ioat callbacks.
> >>>
> >>> Fixes: 2aa47e94bfb2 ("examples/vhost: add ioat ring space count and
> >> check")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
> >>> ---
> >>>  examples/vhost/ioat.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c index
> >>> 60b73be93..9cb5e0d50 100644
> >>> --- a/examples/vhost/ioat.c
> >>> +++ b/examples/vhost/ioat.c
> >>> @@ -113,7 +113,7 @@ open_ioat(const char *value)
> >>>  			goto out;
> >>>  		}
> >>>  		rte_rawdev_start(dev_id);
> >>> -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
> >>> +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
> >>
> >> That really comforts me in thinking we need a generic abstraction for
> >> DMA devices. How is the application developer supposed to know that
> >> the DMA driver has such weird limitations?
> >>
> >> Can the driver be fixed to have a proper behavior?
> >
> > Here is the patch of providing capacity check API for DMA:
> > http://patches.dpdk.org/project/dpdk/patch/20210318182042.43658-6-
> bruc
> > e.richardson@intel.com/
> 
> OK, thanks for the pointer.
> 
> While this new API is being reviewed, and for LTS, let's pick your patch. As
> soon as Bruce patch is merged, please send a new patch on top to make use
> of this API.

Sure, thanks a lot.
Cheng

> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime
> 
> > Thanks,
> > Jiayu
> >>
> >>>  		dma_info->nr++;
> >>>  		i++;
> >>>  	}
> >>>
> >
Liang Ma April 18, 2021, 3:10 p.m. UTC | #9 
On Wed, Apr 07, 2021 at 10:43:36AM +0200, Thomas Monjalon wrote:
> +Cc more people to start a discussion about a potential DMA API.
> If you think it is interesting, we can start a fresh discussion thread.
+1 that's ineresting to have a abstract layer of DMA offload engine. 
   I would like join the discussion. please add me in the cc list in the
   new thread.
> 07/04/2021 10:26, Thomas Monjalon:
> > 07/04/2021 09:47, Maxime Coquelin:
> > > 
> > > On 3/17/21 6:40 AM, Cheng Jiang wrote:
> > > > We use ioat ring space for determining if ioat callbacks can enqueue a
> > > > packet to ioat device. But there is one slot can't be used in ioat
> > > > ring due to the ioat driver design, so we need to reduce one slot in
> > > > ioat ring to prevent ring size mismatch in ioat callbacks.
> [...]
> > > > --- a/examples/vhost/ioat.c
> > > > +++ b/examples/vhost/ioat.c
> > > > @@ -113,7 +113,7 @@ open_ioat(const char *value)
> > > >  			goto out;
> > > >  		}
> > > >  		rte_rawdev_start(dev_id);
> > > > -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
> > > > +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
> > > 
> > > That really comforts me in thinking we need a generic abstraction for
> > > DMA devices. How is the application developer supposed to know that
> > > the DMA driver has such weird limitations?
> > 
> > Having a generic DMA API may be interesting.
> > Do you know any other HW candidate for such an API?
> > Do you think rte_memcpy can be used as a SW driver?
> 
> 
>
Chenbo Xia April 28, 2021, 2:09 a.m. UTC | #10 
> -----Original Message-----
> From: Jiang, Cheng1 <cheng1.jiang@intel.com>
> Sent: Wednesday, March 17, 2021 1:41 PM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>; Wang, Yinan <yinan.wang@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>; stable@dpdk.org
> Subject: [PATCH] examples/vhost: fix ioat ring space in callbacks
> 
> We use ioat ring space for determining if ioat callbacks can enqueue a
> packet to ioat device. But there is one slot can't be used in ioat
> ring due to the ioat driver design, so we need to reduce one slot in
> ioat ring to prevent ring size mismatch in ioat callbacks.
> 
> Fixes: 2aa47e94bfb2 ("examples/vhost: add ioat ring space count and check")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
> ---
>  examples/vhost/ioat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c
> index 60b73be93..9cb5e0d50 100644
> --- a/examples/vhost/ioat.c
> +++ b/examples/vhost/ioat.c
> @@ -113,7 +113,7 @@ open_ioat(const char *value)
>  			goto out;
>  		}
>  		rte_rawdev_start(dev_id);
> -		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
> +		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
>  		dma_info->nr++;
>  		i++;
>  	}
> --
> 2.29.2

Patch applied to next-virtio/main, Thanks
diff mbox series

Patch

diff --git a/examples/vhost/ioat.c b/examples/vhost/ioat.c
index 60b73be93..9cb5e0d50 100644
--- a/examples/vhost/ioat.c
+++ b/examples/vhost/ioat.c
@@ -113,7 +113,7 @@  open_ioat(const char *value)
 			goto out;
 		}
 		rte_rawdev_start(dev_id);
-		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE;
+		cb_tracker[dev_id].ioat_space = IOAT_RING_SIZE - 1;
 		dma_info->nr++;
 		i++;
 	}