[dpdk-dev] rte_ring work

[dpdk-dev] rte_ring work

[Honnappa Nagarahalli]

Hi Konstantin,
	Just to make sure we are coordinating the work on rte_ring, I plan to get the following things done for 20.08. Please let me know if you have any comments.

1) Remove experimental tag on rte_ring_xxx_elem APIs - they have been around for 2 releases now.
2) Make the legacy APIs use elem APIs
3) Check the possibility to remove rte_ring_generic.h implementation. This can be done if the legacy API performance between the generic and C11 implementations is not much.
4) Peek APIs for RTS

Some cleanup activity (assuming above things are successful)

1) Remove the detailed comments on top of the internal functions - it is hard to maintain, the parameters are already self-explanatory
3) Files need some re-org
	a) rte_ring.h, rte_ring_hts.h, rte_ring_rts.h, rte_ring_peek.h - will have legacy format APIs written as wrappers around xxx_elem APIs
	b) rte_ring_elem.h, rte_ring_hts_elem.h, rte_ring_rts_elem.h, rte_ring_peek_elem.h - will have xxx_elem APIs
	c) ring_elem_pvt.h, ring_hts_elem_pvt.h, ring_rts_elem_pvt.h, ring_peek_elem_pvt.h
		- these will contain the internal functions including the c11 functions to manipulate the head/tail pointers.
		  The files with xxx_c11_mem.h will disappear. Make sure private functions have __rte prefix

Thank you,
Honnappa

Re: [dpdk-dev] rte_ring work

[Ananyev, Konstantin]

Hi Honnappa,

> 
> Hi Konstantin,
> 	Just to make sure we are coordinating the work on rte_ring, I plan to get the following things done for 20.08. Please let me know if
> you have any comments.

Thanks for sharing your plans.
In general, looks reasonable to me.
Few comments/questions below.
From my side - I have just few rte_ring related plans for 20.08:
1) Try to integrate hts/rts ring mode into mempool_ring (new flags for mempool_create or so).
2) Use peek API into crypto/scheduler PMD. 
3) Plus few ideas how to improve ring performance in particular usage scenarios. 
Don't know yet would it boil to something real or not (might be new sync mode or so).  

> 1) Remove experimental tag on rte_ring_xxx_elem APIs - they have been around for 2 releases now.
> 2) Make the legacy APIs use elem APIs
> 3) Check the possibility to remove rte_ring_generic.h implementation. This can be done if the legacy API performance between the generic
> and C11 implementations is not much.
> 4) Peek APIs for RTS

Due to nature of RTS I don't think it is possible to implement peek API for it.
But might be I am missing something here.

BTW, do you guys plan to continue with WFE API adoption in rte_ring? 

> Some cleanup activity (assuming above things are successful)
> 
> 1) Remove the detailed comments on top of the internal functions - it is hard to maintain, the parameters are already self-explanatory
> 3) Files need some re-org
> 	a) rte_ring.h, rte_ring_hts.h, rte_ring_rts.h, rte_ring_peek.h - will have legacy format APIs written as wrappers around xxx_elem
> APIs
> 	b) rte_ring_elem.h, rte_ring_hts_elem.h, rte_ring_rts_elem.h, rte_ring_peek_elem.h - will have xxx_elem APIs
> 	c) ring_elem_pvt.h, ring_hts_elem_pvt.h, ring_rts_elem_pvt.h, ring_peek_elem_pvt.h
> 		- these will contain the internal functions including the c11 functions to manipulate the head/tail pointers.
> 		  The files with xxx_c11_mem.h will disappear. Make sure private functions have __rte prefix

Basically you'd plan to:
a) rename rte_ring_*_c11_mem.h  to rte_ring_*_pvt.h
b) get rid of rte_ring_generic.h
Correct?

Re: [dpdk-dev] rte_ring work

[Honnappa Nagarahalli]

<snip>

> 
> Hi Honnappa,
> 
> >
> > Hi Konstantin,
> > 	Just to make sure we are coordinating the work on rte_ring, I plan to
> > get the following things done for 20.08. Please let me know if you have any
> comments.
> 
> Thanks for sharing your plans.
> In general, looks reasonable to me.
> Few comments/questions below.
> From my side - I have just few rte_ring related plans for 20.08:
> 1) Try to integrate hts/rts ring mode into mempool_ring (new flags for
> mempool_create or so).
> 2) Use peek API into crypto/scheduler PMD.
> 3) Plus few ideas how to improve ring performance in particular usage
> scenarios.
> Don't know yet would it boil to something real or not (might be new sync
> mode or so).
> 
> > 1) Remove experimental tag on rte_ring_xxx_elem APIs - they have been
> around for 2 releases now.
> > 2) Make the legacy APIs use elem APIs
> > 3) Check the possibility to remove rte_ring_generic.h implementation.
> > This can be done if the legacy API performance between the generic and
> C11 implementations is not much.
> > 4) Peek APIs for RTS
> 
> Due to nature of RTS I don't think it is possible to implement peek API for it.
> But might be I am missing something here.
I think we have talked about it, probably a patch will make it clear.

> 
> BTW, do you guys plan to continue with WFE API adoption in rte_ring?
Yes, we plan to. I thought you might not be interested in it and removed it here. There are new requirements on the rte_wait_until_xxx API due to the new sync modes. We might have to revisit the API definition.

> 
> > Some cleanup activity (assuming above things are successful)
> >
> > 1) Remove the detailed comments on top of the internal functions - it
> > is hard to maintain, the parameters are already self-explanatory
> > 3) Files need some re-org
> > 	a) rte_ring.h, rte_ring_hts.h, rte_ring_rts.h, rte_ring_peek.h - will
> > have legacy format APIs written as wrappers around xxx_elem APIs
> > 	b) rte_ring_elem.h, rte_ring_hts_elem.h, rte_ring_rts_elem.h,
> rte_ring_peek_elem.h - will have xxx_elem APIs
> > 	c) ring_elem_pvt.h, ring_hts_elem_pvt.h, ring_rts_elem_pvt.h,
> ring_peek_elem_pvt.h
> > 		- these will contain the internal functions including the c11
> functions to manipulate the head/tail pointers.
> > 		  The files with xxx_c11_mem.h will disappear. Make sure
> private
> > functions have __rte prefix
> 
> Basically you'd plan to:
> a) rename rte_ring_*_c11_mem.h  to rte_ring_*_pvt.h
> b) get rid of rte_ring_generic.h
> Correct?
Yes

Re: [dpdk-dev] rte_ring work

[Ananyev, Konstantin]

> >
> > Hi Honnappa,
> >
> > >
> > > Hi Konstantin,
> > > 	Just to make sure we are coordinating the work on rte_ring, I plan to
> > > get the following things done for 20.08. Please let me know if you have any
> > comments.
> >
> > Thanks for sharing your plans.
> > In general, looks reasonable to me.
> > Few comments/questions below.
> > From my side - I have just few rte_ring related plans for 20.08:
> > 1) Try to integrate hts/rts ring mode into mempool_ring (new flags for
> > mempool_create or so).
> > 2) Use peek API into crypto/scheduler PMD.
> > 3) Plus few ideas how to improve ring performance in particular usage
> > scenarios.
> > Don't know yet would it boil to something real or not (might be new sync
> > mode or so).
> >
> > > 1) Remove experimental tag on rte_ring_xxx_elem APIs - they have been
> > around for 2 releases now.
> > > 2) Make the legacy APIs use elem APIs
> > > 3) Check the possibility to remove rte_ring_generic.h implementation.
> > > This can be done if the legacy API performance between the generic and
> > C11 implementations is not much.
> > > 4) Peek APIs for RTS
> >
> > Due to nature of RTS I don't think it is possible to implement peek API for it.
> > But might be I am missing something here.
> I think we have talked about it, probably a patch will make it clear.

Ok, will wait for the patch then.
 
> >
> > BTW, do you guys plan to continue with WFE API adoption in rte_ring?
> Yes, we plan to. I thought you might not be interested in it and removed it here. There are new requirements on the rte_wait_until_xxx API
> due to the new sync modes. We might have to revisit the API definition.
> 
> >
> > > Some cleanup activity (assuming above things are successful)
> > >
> > > 1) Remove the detailed comments on top of the internal functions - it
> > > is hard to maintain, the parameters are already self-explanatory
> > > 3) Files need some re-org
> > > 	a) rte_ring.h, rte_ring_hts.h, rte_ring_rts.h, rte_ring_peek.h - will
> > > have legacy format APIs written as wrappers around xxx_elem APIs
> > > 	b) rte_ring_elem.h, rte_ring_hts_elem.h, rte_ring_rts_elem.h,
> > rte_ring_peek_elem.h - will have xxx_elem APIs
> > > 	c) ring_elem_pvt.h, ring_hts_elem_pvt.h, ring_rts_elem_pvt.h,
> > ring_peek_elem_pvt.h
> > > 		- these will contain the internal functions including the c11
> > functions to manipulate the head/tail pointers.
> > > 		  The files with xxx_c11_mem.h will disappear. Make sure
> > private
> > > functions have __rte prefix
> >
> > Basically you'd plan to:
> > a) rename rte_ring_*_c11_mem.h  to rte_ring_*_pvt.h
> > b) get rid of rte_ring_generic.h
> > Correct?
> Yes

If there would be no perf drops, I have no objections.
Though recently there was a discussion is it ok to remove
dpdk installable headers (even ones marked as internal).    

Re: [dpdk-dev] rte_ring work

[Honnappa Nagarahalli]

> > >
> > > Hi Honnappa,
> > >
> > > >
> > > > Hi Konstantin,
> > > > 	Just to make sure we are coordinating the work on rte_ring, I
> > > > plan to get the following things done for 20.08. Please let me
> > > > know if you have any
> > > comments.
> > >
> > > Thanks for sharing your plans.
> > > In general, looks reasonable to me.
> > > Few comments/questions below.
> > > From my side - I have just few rte_ring related plans for 20.08:
> > > 1) Try to integrate hts/rts ring mode into mempool_ring (new flags
> > > for mempool_create or so).
> > > 2) Use peek API into crypto/scheduler PMD.
> > > 3) Plus few ideas how to improve ring performance in particular
> > > usage scenarios.
> > > Don't know yet would it boil to something real or not (might be new
> > > sync mode or so).
> > >
> > > > 1) Remove experimental tag on rte_ring_xxx_elem APIs - they have
> > > > been
> > > around for 2 releases now.
> > > > 2) Make the legacy APIs use elem APIs
> > > > 3) Check the possibility to remove rte_ring_generic.h implementation.
> > > > This can be done if the legacy API performance between the generic
> > > > and
> > > C11 implementations is not much.
> > > > 4) Peek APIs for RTS
> > >
> > > Due to nature of RTS I don't think it is possible to implement peek API for it.
> > > But might be I am missing something here.
> > I think we have talked about it, probably a patch will make it clear.
> 
> Ok, will wait for the patch then.
> 
> > >
> > > BTW, do you guys plan to continue with WFE API adoption in rte_ring?
> > Yes, we plan to. I thought you might not be interested in it and
> > removed it here. There are new requirements on the rte_wait_until_xxx API
> due to the new sync modes. We might have to revisit the API definition.
> >
> > >
> > > > Some cleanup activity (assuming above things are successful)
> > > >
> > > > 1) Remove the detailed comments on top of the internal functions -
> > > > it is hard to maintain, the parameters are already
> > > > self-explanatory
> > > > 3) Files need some re-org
> > > > 	a) rte_ring.h, rte_ring_hts.h, rte_ring_rts.h, rte_ring_peek.h -
> > > > will have legacy format APIs written as wrappers around xxx_elem APIs
> > > > 	b) rte_ring_elem.h, rte_ring_hts_elem.h, rte_ring_rts_elem.h,
> > > rte_ring_peek_elem.h - will have xxx_elem APIs
> > > > 	c) ring_elem_pvt.h, ring_hts_elem_pvt.h, ring_rts_elem_pvt.h,
> > > ring_peek_elem_pvt.h
> > > > 		- these will contain the internal functions including the c11
> > > functions to manipulate the head/tail pointers.
> > > > 		  The files with xxx_c11_mem.h will disappear. Make sure
> > > private
> > > > functions have __rte prefix
> > >
> > > Basically you'd plan to:
> > > a) rename rte_ring_*_c11_mem.h  to rte_ring_*_pvt.h
> > > b) get rid of rte_ring_generic.h
> > > Correct?
> > Yes
> 
> If there would be no perf drops, I have no objections.
Agree
> Though recently there was a discussion is it ok to remove
> dpdk installable headers (even ones marked as internal).
Do you remember any conclusions? I tried to search, could not find the discussion.

Re: [dpdk-dev] rte_ring work

[Ananyev, Konstantin]

> > > > > Some cleanup activity (assuming above things are successful)
> > > > >
> > > > > 1) Remove the detailed comments on top of the internal functions -
> > > > > it is hard to maintain, the parameters are already
> > > > > self-explanatory
> > > > > 3) Files need some re-org
> > > > > 	a) rte_ring.h, rte_ring_hts.h, rte_ring_rts.h, rte_ring_peek.h -
> > > > > will have legacy format APIs written as wrappers around xxx_elem APIs
> > > > > 	b) rte_ring_elem.h, rte_ring_hts_elem.h, rte_ring_rts_elem.h,
> > > > rte_ring_peek_elem.h - will have xxx_elem APIs
> > > > > 	c) ring_elem_pvt.h, ring_hts_elem_pvt.h, ring_rts_elem_pvt.h,
> > > > ring_peek_elem_pvt.h
> > > > > 		- these will contain the internal functions including the c11
> > > > functions to manipulate the head/tail pointers.
> > > > > 		  The files with xxx_c11_mem.h will disappear. Make sure
> > > > private
> > > > > functions have __rte prefix
> > > >
> > > > Basically you'd plan to:
> > > > a) rename rte_ring_*_c11_mem.h  to rte_ring_*_pvt.h
> > > > b) get rid of rte_ring_generic.h
> > > > Correct?
> > > Yes
> >
> > If there would be no perf drops, I have no objections.
> Agree
> > Though recently there was a discussion is it ok to remove
> > dpdk installable headers (even ones marked as internal).
> Do you remember any conclusions? I tried to search, could not find the discussion.

http://patches.dpdk.org/patch/69560/

Re: [dpdk-dev] rte_ring work

[Honnappa Nagarahalli]

> > > > > > Some cleanup activity (assuming above things are successful)
> > > > > >
> > > > > > 1) Remove the detailed comments on top of the internal
> > > > > > functions - it is hard to maintain, the parameters are already
> > > > > > self-explanatory
> > > > > > 3) Files need some re-org
> > > > > > 	a) rte_ring.h, rte_ring_hts.h, rte_ring_rts.h,
> > > > > > rte_ring_peek.h - will have legacy format APIs written as wrappers
> around xxx_elem APIs
> > > > > > 	b) rte_ring_elem.h, rte_ring_hts_elem.h, rte_ring_rts_elem.h,
> > > > > rte_ring_peek_elem.h - will have xxx_elem APIs
> > > > > > 	c) ring_elem_pvt.h, ring_hts_elem_pvt.h, ring_rts_elem_pvt.h,
> > > > > ring_peek_elem_pvt.h
> > > > > > 		- these will contain the internal functions including
> the
> > > > > > c11
> > > > > functions to manipulate the head/tail pointers.
> > > > > > 		  The files with xxx_c11_mem.h will disappear. Make
> sure
> > > > > private
> > > > > > functions have __rte prefix
> > > > >
> > > > > Basically you'd plan to:
> > > > > a) rename rte_ring_*_c11_mem.h  to rte_ring_*_pvt.h
> > > > > b) get rid of rte_ring_generic.h Correct?
> > > > Yes
> > >
> > > If there would be no perf drops, I have no objections.
> > Agree
> > > Though recently there was a discussion is it ok to remove dpdk
> > > installable headers (even ones marked as internal).
> > Do you remember any conclusions? I tried to search, could not find the
> discussion.
> 
> http://patches.dpdk.org/patch/69560/
Thank you. rte_ring library has called out clearly if a particular file should be included or not. If the users have included other files despite that, may be DPDK should not be held accountable?

Re: [dpdk-dev] rte_ring work

[Ananyev, Konstantin]

> > > > > > > Some cleanup activity (assuming above things are successful)
> > > > > > >
> > > > > > > 1) Remove the detailed comments on top of the internal
> > > > > > > functions - it is hard to maintain, the parameters are already
> > > > > > > self-explanatory
> > > > > > > 3) Files need some re-org
> > > > > > > 	a) rte_ring.h, rte_ring_hts.h, rte_ring_rts.h,
> > > > > > > rte_ring_peek.h - will have legacy format APIs written as wrappers
> > around xxx_elem APIs
> > > > > > > 	b) rte_ring_elem.h, rte_ring_hts_elem.h, rte_ring_rts_elem.h,
> > > > > > rte_ring_peek_elem.h - will have xxx_elem APIs
> > > > > > > 	c) ring_elem_pvt.h, ring_hts_elem_pvt.h, ring_rts_elem_pvt.h,
> > > > > > ring_peek_elem_pvt.h
> > > > > > > 		- these will contain the internal functions including
> > the
> > > > > > > c11
> > > > > > functions to manipulate the head/tail pointers.
> > > > > > > 		  The files with xxx_c11_mem.h will disappear. Make
> > sure
> > > > > > private
> > > > > > > functions have __rte prefix
> > > > > >
> > > > > > Basically you'd plan to:
> > > > > > a) rename rte_ring_*_c11_mem.h  to rte_ring_*_pvt.h
> > > > > > b) get rid of rte_ring_generic.h Correct?
> > > > > Yes
> > > >
> > > > If there would be no perf drops, I have no objections.
> > > Agree
> > > > Though recently there was a discussion is it ok to remove dpdk
> > > > installable headers (even ones marked as internal).
> > > Do you remember any conclusions? I tried to search, could not find the
> > discussion.
> >
> > http://patches.dpdk.org/patch/69560/
> Thank you. rte_ring library has called out clearly if a particular file should be included or not. If the users have included other files despite
> that, may be DPDK should not be held accountable?

Agree.