[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
On 2015/02/18 21:33, Iremonger, Bernard wrote: > >> -Original Message- >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tetsuya Mukawa >> Sent: Wednesday, February 18, 2015 10:58 AM >> To: Richardson, Bruce; Thomas Monjalon >> Cc: dev at dpdk.org; Neil Horman >> Subject: Re: [dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption >> that port will not be >> detached >> >> On 2015/02/18 19:03, Bruce Richardson wrote: >>> On Wed, Feb 18, 2015 at 10:57:25AM +0100, Thomas Monjalon wrote: >>>> 2015-02-18 15:10, Tetsuya Mukawa: >>>>> On 2015/02/18 10:54, Tetsuya Mukawa wrote: >>>>>> On 2015/02/18 9:31, Thomas Monjalon wrote: >>>>>>> 2015-02-17 15:14, Tetsuya Mukawa: >>>>>>>> On 2015/02/17 9:36, Thomas Monjalon wrote: >>>>>>>>> 2015-02-16 13:14, Tetsuya Mukawa: >>>>>>>>> Is uint8_t sill a good size for hotpluggable virtual device ids? >>>>>>>> I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" >>>>>>>> as port id. >>>>>>>> If someone reports it doesn't enough, I guess it will be the time >>>>>>>> to write a patch to change all uint_8 in one patch. >>>>>>> It's a big ABI breakage. So if we feel it's going to be required, >>>>>>> it's better to do it now in 2.0 release I think. >>>>>>> >>>>>>> Any opinion? >>>>>>> >>>>>> Hi Thomas, >>>>>> >>>>>> I agree with it. >>>>>> I will add an one more patch to change uint8_t to uint16_t. >>>>>> >>>>>> Thanks, >>>>>> Tetsuya >>>>>> >>>>> Hi Thomas, >>>>> >>>>> Could I make sure. >>>>> After changing uint8_t to uint16_t in "rte_ethdev.[ch]", must I also >>>>> need to change other applications and libraries that call ethdev APIs? >>>>> If so, I would not finish it by 23rd. >>>>> >>>>> I've counted how many lines call ethdev APIs that are related to port_id. >>>>> Could you please check an attached file? >>>>> It's over 1200 lines. Probably to fix one of caller, I will need to >>>>> check how port_id is used, and fix more related lines. So probably >>>>> thousands lines may need to be fixed. >>>>> >>>>> When is deadline for fixing this changing? >>>>> Also, if you have a good idea to fix it easier, could you please let >>>>> me know? >>>> It was an open question. >>>> If everybody is fine with 255 ports maximum, let's keep it as is. >>>> >>> I think we are probably ok for now (and forseeable future) with 255 max. >>> >>> However, if we did change it, I agree that in 2.0 is a very good time to do >>> so. >>> Since we are expanding the field, rather than shrinking it, I don't >>> see why we can't just make the change at the ethdev level (and in libs >>> API) in 2.0 and then in later releases (e.g. 2.1) update the apps and >>> examples to match. That way the ABI stays the same from 2.0 onwards, >>> and we don't have a huge amount of churn changing it everywhere late in the >>> 2.0 release cycle. >> Hi Bruce, >> >> Could you please check my RFC patch I will send soon? >> I wrote the patch like below. >> >> 1. Copy header file like below. >> $ cp lib/librte_ether/rte_ethdev.h lib/librte_ether/rte_ethdev_internal.h >> 2. Change "rte_ethdev.c" to include "rte_ethdev_internal.h" >> 3. Change type of port id in "rte_ethdev.c" and "rte_ethdev_internal.h". >> >> If the patch is OK, I wll send it with hotplug patches. >> >> Thanks, >> Tetsuya >> >> >>> /Bruce > Hi Tetsuya, > > After this change there will be two header files with a lot of the same > information. > lib/librte_ether/rte_ethdev.h > lib/librte_ether/rte_ethdev_internal.h > I don't think this is a good idea for maintenance in the future. > If 255 is ok for the foreseeable future, why change it now. Hi Bernard, I appreciate for your checking. Agree, it will not be good to have almost same headers. Thanks, Tetsuya > Regards, > > Bernard. > >
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
On 2015/02/18 21:23, Bruce Richardson wrote: > On Wed, Feb 18, 2015 at 07:58:06PM +0900, Tetsuya Mukawa wrote: >> On 2015/02/18 19:03, Bruce Richardson wrote: >>> On Wed, Feb 18, 2015 at 10:57:25AM +0100, Thomas Monjalon wrote: 2015-02-18 15:10, Tetsuya Mukawa: > On 2015/02/18 10:54, Tetsuya Mukawa wrote: >> On 2015/02/18 9:31, Thomas Monjalon wrote: >>> 2015-02-17 15:14, Tetsuya Mukawa: On 2015/02/17 9:36, Thomas Monjalon wrote: > 2015-02-16 13:14, Tetsuya Mukawa: > Is uint8_t sill a good size for hotpluggable virtual device ids? I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" as port id. If someone reports it doesn't enough, I guess it will be the time to write a patch to change all uint_8 in one patch. >>> It's a big ABI breakage. So if we feel it's going to be required, >>> it's better to do it now in 2.0 release I think. >>> >>> Any opinion? >>> >> Hi Thomas, >> >> I agree with it. >> I will add an one more patch to change uint8_t to uint16_t. >> >> Thanks, >> Tetsuya >> > Hi Thomas, > > Could I make sure. > After changing uint8_t to uint16_t in "rte_ethdev.[ch]", must I also > need to change other applications and libraries that call ethdev APIs? > If so, I would not finish it by 23rd. > > I've counted how many lines call ethdev APIs that are related to port_id. > Could you please check an attached file? > It's over 1200 lines. Probably to fix one of caller, I will need to > check how port_id is used, and fix more related lines. So probably > thousands lines may need to be fixed. > > When is deadline for fixing this changing? > Also, if you have a good idea to fix it easier, could you please let me > know? It was an open question. If everybody is fine with 255 ports maximum, let's keep it as is. >>> I think we are probably ok for now (and forseeable future) with 255 max. >>> >>> However, if we did change it, I agree that in 2.0 is a very good time to do >>> so. >>> Since we are expanding the field, rather than shrinking it, I don't see why >>> we >>> can't just make the change at the ethdev level (and in libs API) in 2.0 and >>> then in >>> later releases (e.g. 2.1) update the apps and examples to match. That way >>> the >>> ABI stays the same from 2.0 onwards, and we don't have a huge amount of >>> churn >>> changing it everywhere late in the 2.0 release cycle. >> Hi Bruce, >> >> Could you please check my RFC patch I will send soon? >> I wrote the patch like below. >> >> 1. Copy header file like below. >> $ cp lib/librte_ether/rte_ethdev.h lib/librte_ether/rte_ethdev_internal.h >> 2. Change "rte_ethdev.c" to include "rte_ethdev_internal.h" >> 3. Change type of port id in "rte_ethdev.c" and "rte_ethdev_internal.h". >> >> If the patch is OK, I wll send it with hotplug patches. >> >> Thanks, >> Tetsuya >> >> > Why the new ethdev internal file? I guess some libraries that include "rte_ethdev.h". To compile these libraries, I thought such a header was needed. But, it seems it's not the time to change type of port_id. I appreciate for your checking. Tetsuya
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
On 2015/02/18 19:03, Bruce Richardson wrote: > On Wed, Feb 18, 2015 at 10:57:25AM +0100, Thomas Monjalon wrote: >> 2015-02-18 15:10, Tetsuya Mukawa: >>> On 2015/02/18 10:54, Tetsuya Mukawa wrote: On 2015/02/18 9:31, Thomas Monjalon wrote: > 2015-02-17 15:14, Tetsuya Mukawa: >> On 2015/02/17 9:36, Thomas Monjalon wrote: >>> 2015-02-16 13:14, Tetsuya Mukawa: >>> Is uint8_t sill a good size for hotpluggable virtual device ids? >> I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" >> as port id. >> If someone reports it doesn't enough, I guess it will be the time to >> write a patch to change all uint_8 in one patch. > It's a big ABI breakage. So if we feel it's going to be required, > it's better to do it now in 2.0 release I think. > > Any opinion? > Hi Thomas, I agree with it. I will add an one more patch to change uint8_t to uint16_t. Thanks, Tetsuya >>> Hi Thomas, >>> >>> Could I make sure. >>> After changing uint8_t to uint16_t in "rte_ethdev.[ch]", must I also >>> need to change other applications and libraries that call ethdev APIs? >>> If so, I would not finish it by 23rd. >>> >>> I've counted how many lines call ethdev APIs that are related to port_id. >>> Could you please check an attached file? >>> It's over 1200 lines. Probably to fix one of caller, I will need to >>> check how port_id is used, and fix more related lines. So probably >>> thousands lines may need to be fixed. >>> >>> When is deadline for fixing this changing? >>> Also, if you have a good idea to fix it easier, could you please let me >>> know? >> It was an open question. >> If everybody is fine with 255 ports maximum, let's keep it as is. >> > I think we are probably ok for now (and forseeable future) with 255 max. > > However, if we did change it, I agree that in 2.0 is a very good time to do > so. > Since we are expanding the field, rather than shrinking it, I don't see why we > can't just make the change at the ethdev level (and in libs API) in 2.0 and > then in > later releases (e.g. 2.1) update the apps and examples to match. That way the > ABI stays the same from 2.0 onwards, and we don't have a huge amount of churn > changing it everywhere late in the 2.0 release cycle. Hi Bruce, Could you please check my RFC patch I will send soon? I wrote the patch like below. 1. Copy header file like below. $ cp lib/librte_ether/rte_ethdev.h lib/librte_ether/rte_ethdev_internal.h 2. Change "rte_ethdev.c" to include "rte_ethdev_internal.h" 3. Change type of port id in "rte_ethdev.c" and "rte_ethdev_internal.h". If the patch is OK, I wll send it with hotplug patches. Thanks, Tetsuya > /Bruce
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
On 2015/02/18 10:54, Tetsuya Mukawa wrote: > On 2015/02/18 9:31, Thomas Monjalon wrote: >> 2015-02-17 15:14, Tetsuya Mukawa: >>> On 2015/02/17 9:36, Thomas Monjalon wrote: 2015-02-16 13:14, Tetsuya Mukawa: Is uint8_t sill a good size for hotpluggable virtual device ids? >>> I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" >>> as port id. >>> If someone reports it doesn't enough, I guess it will be the time to >>> write a patch to change all uint_8 in one patch. >> It's a big ABI breakage. So if we feel it's going to be required, >> it's better to do it now in 2.0 release I think. >> >> Any opinion? >> > Hi Thomas, > > I agree with it. > I will add an one more patch to change uint8_t to uint16_t. > > Thanks, > Tetsuya > Hi Thomas, Could I make sure. After changing uint8_t to uint16_t in "rte_ethdev.[ch]", must I also need to change other applications and libraries that call ethdev APIs? If so, I would not finish it by 23rd. I've counted how many lines call ethdev APIs that are related to port_id. Could you please check an attached file? It's over 1200 lines. Probably to fix one of caller, I will need to check how port_id is used, and fix more related lines. So probably thousands lines may need to be fixed. When is deadline for fixing this changing? Also, if you have a good idea to fix it easier, could you please let me know? Thanks, Tetsuya -- next part -- rte_eth_dev_configure app/test-pipeline/init.c240 rte_eth_dev_configure app/test-pmd/testpmd.c 1304 rte_eth_dev_configure app/test/test_kni.c 523 rte_eth_dev_configure app/test/test_link_bonding.c238 rte_eth_dev_configure app/test/test_link_bonding.c240 rte_eth_dev_configure app/test/test_pmd_perf.c751 rte_eth_dev_configure app/test/test_pmd_ring.c67 rte_eth_dev_configure app/test/test_pmd_ring.c73 rte_eth_dev_configure app/test/test_pmd_ring.c77 rte_eth_dev_configure app/test/test_pmd_ring.c81 rte_eth_dev_configure app/test/test_pmd_ring.c256 rte_eth_dev_configure app/test/test_pmd_ring.c257 rte_eth_dev_configure examples/distributor/main.c 125 rte_eth_dev_configure examples/dpdk_qat/main.c726 rte_eth_dev_configure examples/exception_path/main.c 433 rte_eth_dev_configure examples/ip_fragmentation/main.c890 rte_eth_dev_configure examples/ip_pipeline/init.c 486 rte_eth_dev_configure examples/ip_reassembly/main.c 1095 rte_eth_dev_configure examples/ipv4_multicast/main.c 755 rte_eth_dev_configure examples/kni/main.c 617 rte_eth_dev_configure examples/kni/main.c 725 rte_eth_dev_configure examples/l2fwd-ivshmem/host/host.c 745 rte_eth_dev_configure examples/l2fwd/main.c 650 rte_eth_dev_configure examples/l3fwd-acl/main.c 1991 rte_eth_dev_configure examples/l3fwd-power/main.c 1534 rte_eth_dev_configure examples/l3fwd-vf/main.c1013 rte_eth_dev_configure examples/l3fwd/main.c 2457 rte_eth_dev_configure examples/link_status_interrupt/main.c 696 rte_eth_dev_configure examples/link_status_interrupt/main.c 702 rte_eth_dev_configure examples/load_balancer/init.c 446 rte_eth_dev_configure examples/multi_process/client_server_mp/mp_server/init.c144 rte_eth_dev_configure examples/multi_process/l2fwd_fork/main.c1121 rte_eth_dev_configure examples/multi_process/symmetric_mp/main.c 248 rte_eth_dev_configure examples/netmap_compat/lib/compat_netmap.c 706 rte_eth_dev_configure examples/qos_meter/main.c 370 rte_eth_dev_configure examples/qos_meter/main.c 386 rte_eth_dev_configure examples/qos_sched/init.c 129 rte_eth_dev_configure examples/quota_watermark/qw/init.c 82 rte_eth_dev_configure examples/skeleton/basicfwd.c69 rte_eth_dev_configure examples/vhost/main.c 442 rte_eth_dev_configure examples/vhost_xen/main.c 306 rte_eth_dev_configure examples/vmdq/main.c254 rte_eth_dev_configure examples/vmdq_dcb/main.c177 rte_eth_dev_configure lib/librte_ether/rte_ethdev.c 728 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 91 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 102 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 1726 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 1744 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 1783 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 1844 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 1860 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 1877 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 1893 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 2112 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 2133 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 2225 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 2377 rte_eth_dev_configure lib/librte_ether/rte_ethdev.h 2492
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tetsuya Mukawa > Sent: Wednesday, February 18, 2015 10:58 AM > To: Richardson, Bruce; Thomas Monjalon > Cc: dev at dpdk.org; Neil Horman > Subject: Re: [dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption > that port will not be > detached > > On 2015/02/18 19:03, Bruce Richardson wrote: > > On Wed, Feb 18, 2015 at 10:57:25AM +0100, Thomas Monjalon wrote: > >> 2015-02-18 15:10, Tetsuya Mukawa: > >>> On 2015/02/18 10:54, Tetsuya Mukawa wrote: > >>>> On 2015/02/18 9:31, Thomas Monjalon wrote: > >>>>> 2015-02-17 15:14, Tetsuya Mukawa: > >>>>>> On 2015/02/17 9:36, Thomas Monjalon wrote: > >>>>>>> 2015-02-16 13:14, Tetsuya Mukawa: > >>>>>>> Is uint8_t sill a good size for hotpluggable virtual device ids? > >>>>>> I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" > >>>>>> as port id. > >>>>>> If someone reports it doesn't enough, I guess it will be the time > >>>>>> to write a patch to change all uint_8 in one patch. > >>>>> It's a big ABI breakage. So if we feel it's going to be required, > >>>>> it's better to do it now in 2.0 release I think. > >>>>> > >>>>> Any opinion? > >>>>> > >>>> Hi Thomas, > >>>> > >>>> I agree with it. > >>>> I will add an one more patch to change uint8_t to uint16_t. > >>>> > >>>> Thanks, > >>>> Tetsuya > >>>> > >>> Hi Thomas, > >>> > >>> Could I make sure. > >>> After changing uint8_t to uint16_t in "rte_ethdev.[ch]", must I also > >>> need to change other applications and libraries that call ethdev APIs? > >>> If so, I would not finish it by 23rd. > >>> > >>> I've counted how many lines call ethdev APIs that are related to port_id. > >>> Could you please check an attached file? > >>> It's over 1200 lines. Probably to fix one of caller, I will need to > >>> check how port_id is used, and fix more related lines. So probably > >>> thousands lines may need to be fixed. > >>> > >>> When is deadline for fixing this changing? > >>> Also, if you have a good idea to fix it easier, could you please let > >>> me know? > >> It was an open question. > >> If everybody is fine with 255 ports maximum, let's keep it as is. > >> > > I think we are probably ok for now (and forseeable future) with 255 max. > > > > However, if we did change it, I agree that in 2.0 is a very good time to do > > so. > > Since we are expanding the field, rather than shrinking it, I don't > > see why we can't just make the change at the ethdev level (and in libs > > API) in 2.0 and then in later releases (e.g. 2.1) update the apps and > > examples to match. That way the ABI stays the same from 2.0 onwards, > > and we don't have a huge amount of churn changing it everywhere late in the > > 2.0 release cycle. > > Hi Bruce, > > Could you please check my RFC patch I will send soon? > I wrote the patch like below. > > 1. Copy header file like below. > $ cp lib/librte_ether/rte_ethdev.h lib/librte_ether/rte_ethdev_internal.h > 2. Change "rte_ethdev.c" to include "rte_ethdev_internal.h" > 3. Change type of port id in "rte_ethdev.c" and "rte_ethdev_internal.h". > > If the patch is OK, I wll send it with hotplug patches. > > Thanks, > Tetsuya > > > > /Bruce > Hi Tetsuya, After this change there will be two header files with a lot of the same information. lib/librte_ether/rte_ethdev.h lib/librte_ether/rte_ethdev_internal.h I don't think this is a good idea for maintenance in the future. If 255 is ok for the foreseeable future, why change it now. Regards, Bernard.
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
On Wed, Feb 18, 2015 at 07:58:06PM +0900, Tetsuya Mukawa wrote: > On 2015/02/18 19:03, Bruce Richardson wrote: > > On Wed, Feb 18, 2015 at 10:57:25AM +0100, Thomas Monjalon wrote: > >> 2015-02-18 15:10, Tetsuya Mukawa: > >>> On 2015/02/18 10:54, Tetsuya Mukawa wrote: > On 2015/02/18 9:31, Thomas Monjalon wrote: > > 2015-02-17 15:14, Tetsuya Mukawa: > >> On 2015/02/17 9:36, Thomas Monjalon wrote: > >>> 2015-02-16 13:14, Tetsuya Mukawa: > >>> Is uint8_t sill a good size for hotpluggable virtual device ids? > >> I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" > >> as port id. > >> If someone reports it doesn't enough, I guess it will be the time to > >> write a patch to change all uint_8 in one patch. > > It's a big ABI breakage. So if we feel it's going to be required, > > it's better to do it now in 2.0 release I think. > > > > Any opinion? > > > Hi Thomas, > > I agree with it. > I will add an one more patch to change uint8_t to uint16_t. > > Thanks, > Tetsuya > > >>> Hi Thomas, > >>> > >>> Could I make sure. > >>> After changing uint8_t to uint16_t in "rte_ethdev.[ch]", must I also > >>> need to change other applications and libraries that call ethdev APIs? > >>> If so, I would not finish it by 23rd. > >>> > >>> I've counted how many lines call ethdev APIs that are related to port_id. > >>> Could you please check an attached file? > >>> It's over 1200 lines. Probably to fix one of caller, I will need to > >>> check how port_id is used, and fix more related lines. So probably > >>> thousands lines may need to be fixed. > >>> > >>> When is deadline for fixing this changing? > >>> Also, if you have a good idea to fix it easier, could you please let me > >>> know? > >> It was an open question. > >> If everybody is fine with 255 ports maximum, let's keep it as is. > >> > > I think we are probably ok for now (and forseeable future) with 255 max. > > > > However, if we did change it, I agree that in 2.0 is a very good time to do > > so. > > Since we are expanding the field, rather than shrinking it, I don't see why > > we > > can't just make the change at the ethdev level (and in libs API) in 2.0 and > > then in > > later releases (e.g. 2.1) update the apps and examples to match. That way > > the > > ABI stays the same from 2.0 onwards, and we don't have a huge amount of > > churn > > changing it everywhere late in the 2.0 release cycle. > > Hi Bruce, > > Could you please check my RFC patch I will send soon? > I wrote the patch like below. > > 1. Copy header file like below. > $ cp lib/librte_ether/rte_ethdev.h lib/librte_ether/rte_ethdev_internal.h > 2. Change "rte_ethdev.c" to include "rte_ethdev_internal.h" > 3. Change type of port id in "rte_ethdev.c" and "rte_ethdev_internal.h". > > If the patch is OK, I wll send it with hotplug patches. > > Thanks, > Tetsuya > > Why the new ethdev internal file?
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
2015-02-18 15:10, Tetsuya Mukawa: > On 2015/02/18 10:54, Tetsuya Mukawa wrote: > > On 2015/02/18 9:31, Thomas Monjalon wrote: > >> 2015-02-17 15:14, Tetsuya Mukawa: > >>> On 2015/02/17 9:36, Thomas Monjalon wrote: > 2015-02-16 13:14, Tetsuya Mukawa: > Is uint8_t sill a good size for hotpluggable virtual device ids? > >>> I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" > >>> as port id. > >>> If someone reports it doesn't enough, I guess it will be the time to > >>> write a patch to change all uint_8 in one patch. > >> It's a big ABI breakage. So if we feel it's going to be required, > >> it's better to do it now in 2.0 release I think. > >> > >> Any opinion? > >> > > Hi Thomas, > > > > I agree with it. > > I will add an one more patch to change uint8_t to uint16_t. > > > > Thanks, > > Tetsuya > > > > Hi Thomas, > > Could I make sure. > After changing uint8_t to uint16_t in "rte_ethdev.[ch]", must I also > need to change other applications and libraries that call ethdev APIs? > If so, I would not finish it by 23rd. > > I've counted how many lines call ethdev APIs that are related to port_id. > Could you please check an attached file? > It's over 1200 lines. Probably to fix one of caller, I will need to > check how port_id is used, and fix more related lines. So probably > thousands lines may need to be fixed. > > When is deadline for fixing this changing? > Also, if you have a good idea to fix it easier, could you please let me > know? It was an open question. If everybody is fine with 255 ports maximum, let's keep it as is.
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
On Wed, Feb 18, 2015 at 10:57:25AM +0100, Thomas Monjalon wrote: > 2015-02-18 15:10, Tetsuya Mukawa: > > On 2015/02/18 10:54, Tetsuya Mukawa wrote: > > > On 2015/02/18 9:31, Thomas Monjalon wrote: > > >> 2015-02-17 15:14, Tetsuya Mukawa: > > >>> On 2015/02/17 9:36, Thomas Monjalon wrote: > > 2015-02-16 13:14, Tetsuya Mukawa: > > Is uint8_t sill a good size for hotpluggable virtual device ids? > > >>> I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" > > >>> as port id. > > >>> If someone reports it doesn't enough, I guess it will be the time to > > >>> write a patch to change all uint_8 in one patch. > > >> It's a big ABI breakage. So if we feel it's going to be required, > > >> it's better to do it now in 2.0 release I think. > > >> > > >> Any opinion? > > >> > > > Hi Thomas, > > > > > > I agree with it. > > > I will add an one more patch to change uint8_t to uint16_t. > > > > > > Thanks, > > > Tetsuya > > > > > > > Hi Thomas, > > > > Could I make sure. > > After changing uint8_t to uint16_t in "rte_ethdev.[ch]", must I also > > need to change other applications and libraries that call ethdev APIs? > > If so, I would not finish it by 23rd. > > > > I've counted how many lines call ethdev APIs that are related to port_id. > > Could you please check an attached file? > > It's over 1200 lines. Probably to fix one of caller, I will need to > > check how port_id is used, and fix more related lines. So probably > > thousands lines may need to be fixed. > > > > When is deadline for fixing this changing? > > Also, if you have a good idea to fix it easier, could you please let me > > know? > > It was an open question. > If everybody is fine with 255 ports maximum, let's keep it as is. > I think we are probably ok for now (and forseeable future) with 255 max. However, if we did change it, I agree that in 2.0 is a very good time to do so. Since we are expanding the field, rather than shrinking it, I don't see why we can't just make the change at the ethdev level (and in libs API) in 2.0 and then in later releases (e.g. 2.1) update the apps and examples to match. That way the ABI stays the same from 2.0 onwards, and we don't have a huge amount of churn changing it everywhere late in the 2.0 release cycle. /Bruce
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tetsuya Mukawa > Sent: Wednesday, February 18, 2015 6:10 AM > To: Thomas Monjalon > Cc: dev at dpdk.org; Neil Horman > Subject: Re: [dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption > that port will not be > detached > > On 2015/02/18 10:54, Tetsuya Mukawa wrote: > > On 2015/02/18 9:31, Thomas Monjalon wrote: > >> 2015-02-17 15:14, Tetsuya Mukawa: > >>> On 2015/02/17 9:36, Thomas Monjalon wrote: > >>>> 2015-02-16 13:14, Tetsuya Mukawa: > >>>> Is uint8_t sill a good size for hotpluggable virtual device ids? > >>> I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" > >>> as port id. > >>> If someone reports it doesn't enough, I guess it will be the time to > >>> write a patch to change all uint_8 in one patch. > >> It's a big ABI breakage. So if we feel it's going to be required, > >> it's better to do it now in 2.0 release I think. > >> > >> Any opinion? > >> > > Hi Thomas, > > > > I agree with it. > > I will add an one more patch to change uint8_t to uint16_t. > > > > Thanks, > > Tetsuya > > > > Hi Thomas, > > Could I make sure. > After changing uint8_t to uint16_t in "rte_ethdev.[ch]", must I also need to > change other applications > and libraries that call ethdev APIs? > If so, I would not finish it by 23rd. > > I've counted how many lines call ethdev APIs that are related to port_id. > Could you please check an attached file? > It's over 1200 lines. Probably to fix one of caller, I will need to check > how port_id is used, and fix more > related lines. So probably thousands lines may need to be fixed. > > When is deadline for fixing this changing? > Also, if you have a good idea to fix it easier, could you please let me know? > > Thanks, > Tetsuya Hi Tetsuya, Thomas, As uint8_t is already widely used for port_id, I don't think it should be changed in this patchset. If it is to be changed to uint16_t it should be done as a separate task (in a new patchset). Regards, Bernard.
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
2015-02-17 15:14, Tetsuya Mukawa: > On 2015/02/17 9:36, Thomas Monjalon wrote: > > 2015-02-16 13:14, Tetsuya Mukawa: > > Is uint8_t sill a good size for hotpluggable virtual device ids? > > I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c" > as port id. > If someone reports it doesn't enough, I guess it will be the time to > write a patch to change all uint_8 in one patch. It's a big ABI breakage. So if we feel it's going to be required, it's better to do it now in 2.0 release I think. Any opinion?
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
Hi Thomas,
I appreciate for your all comments.
On 2015/02/17 9:36, Thomas Monjalon wrote:
> 2015-02-16 13:14, Tetsuya Mukawa:
>> To remove assumption, do like followings.
>>
>> This patch adds "RTE_PCI_DRV_DETACHABLE" to drv_flags of rte_pci_driver
>> structure. The flags indicate the driver can detach devices at runtime.
>> Also, remove assumption that port will not be detached.
>>
>> To remove the assumption.
>> - Add 'attached' member to rte_eth_dev structure.
>> This member is used for indicating the port is attached, or not.
>> - Add rte_eth_dev_allocate_new_port().
>> This function is used for allocating new port.
>>
>> v8:
>> - NONE_TRACE is changed to NO_TRACE.
>> (Thanks to Iremonger, Bernard)
>> v5:
>> - Change parameters of rte_eth_dev_validate_port() to cleanup code.
>> v4:
>> - Use braces with 'for' loop.
>> - Fix indent of 'if' statement.
>>
>> Signed-off-by: Tetsuya Mukawa
>> ---
>> lib/librte_eal/common/include/rte_pci.h | 2 +
>> lib/librte_ether/rte_ethdev.c | 454
>> +---
>> lib/librte_ether/rte_ethdev.h | 5 +
>> 3 files changed, 186 insertions(+), 275 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/include/rte_pci.h
>> b/lib/librte_eal/common/include/rte_pci.h
>> index 7b48b55..7f2d699 100644
>> --- a/lib/librte_eal/common/include/rte_pci.h
>> +++ b/lib/librte_eal/common/include/rte_pci.h
>> @@ -207,6 +207,8 @@ struct rte_pci_driver {
>> #define RTE_PCI_DRV_FORCE_UNBIND 0x0004
>> /** Device driver supports link state interrupt */
>> #define RTE_PCI_DRV_INTR_LSC0x0008
>> +/** Device driver supports detaching capability */
>> +#define RTE_PCI_DRV_DETACHABLE 0x0010
>>
>> /**< Internal use only - Macro used by pci addr parsing functions **/
>> #define GET_PCIADDR_FIELD(in, fd, lim, dlm) \
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index ea3a1fb..a79fa5b 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -175,6 +175,16 @@ enum {
>> STAT_QMAP_RX
>> };
>>
>> +enum {
>> +DEV_INVALID = 0,
>> +DEV_VALID,
>> +};
>> +
>> +enum {
>> +DEV_DISCONNECTED = 0,
>> +DEV_CONNECTED
>> +};
> The commit log explains what is an attached port but not what means
> valid or connected.
> These enums should have a comment to explain their usage.
Sure, I will add comment, and fix commit log.
>> +
>> static inline void
>> rte_eth_dev_data_alloc(void)
>> {
>> @@ -201,19 +211,34 @@ rte_eth_dev_allocated(const char *name)
>> {
>> unsigned i;
>>
>> -for (i = 0; i < nb_ports; i++) {
>> -if (strcmp(rte_eth_devices[i].data->name, name) == 0)
>> +for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
>> +if ((rte_eth_devices[i].attached == DEV_CONNECTED) &&
>> +strcmp(rte_eth_devices[i].data->name, name) == 0)
>> return _eth_devices[i];
>> }
>> return NULL;
>> }
>>
>> +static uint8_t
>> +rte_eth_dev_allocate_new_port(void)
>> +{
>> +unsigned i;
>> +
>> +for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
>> +if (rte_eth_devices[i].attached == DEV_DISCONNECTED)
>> +return i;
>> +}
>> +return RTE_MAX_ETHPORTS;
>> +}
> This function does not allocate a new port.
> It get the first free port id.
I will change the function name like below.
rte_eth_dev_find_free_port()
> Is uint8_t sill a good size for hotpluggable virtual device ids?
I am not sure it's enough, but uint8_t is widely used in "rte_ethdev.c"
as port id.
If someone reports it doesn't enough, I guess it will be the time to
write a patch to change all uint_8 in one patch.
>> +
>> struct rte_eth_dev *
>> rte_eth_dev_allocate(const char *name)
>> {
>> +uint8_t port_id;
>> struct rte_eth_dev *eth_dev;
>>
>> -if (nb_ports == RTE_MAX_ETHPORTS) {
>> +port_id = rte_eth_dev_allocate_new_port();
>> +if (port_id == RTE_MAX_ETHPORTS) {
>> PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports\n");
>> return NULL;
>> }
>> @@ -226,10 +251,12 @@ rte_eth_dev_allocate(const char *name)
>> return NULL;
>> }
>>
>> -eth_dev = _eth_devices[nb_ports];
>> -eth_dev->data = _eth_dev_data[nb_ports];
>> +eth_dev = _eth_devices[port_id];
>> +eth_dev->data = _eth_dev_data[port_id];
>> snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
>> -eth_dev->data->port_id = nb_ports++;
>> +eth_dev->data->port_id = port_id;
>> +eth_dev->attached = DEV_CONNECTED;
>> +nb_ports++;
>> return eth_dev;
>> }
>>
>> @@ -283,6 +310,7 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
>> (unsigned) pci_dev->id.device_id);
>> if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>> rte_free(eth_dev->data->dev_private);
>> +eth_dev->attached = DEV_DISCONNECTED;
>> nb_ports--;
>> return diag;
>> }
>> @@
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
2015-02-16 13:14, Tetsuya Mukawa:
> To remove assumption, do like followings.
>
> This patch adds "RTE_PCI_DRV_DETACHABLE" to drv_flags of rte_pci_driver
> structure. The flags indicate the driver can detach devices at runtime.
> Also, remove assumption that port will not be detached.
>
> To remove the assumption.
> - Add 'attached' member to rte_eth_dev structure.
> This member is used for indicating the port is attached, or not.
> - Add rte_eth_dev_allocate_new_port().
> This function is used for allocating new port.
>
> v8:
> - NONE_TRACE is changed to NO_TRACE.
> (Thanks to Iremonger, Bernard)
> v5:
> - Change parameters of rte_eth_dev_validate_port() to cleanup code.
> v4:
> - Use braces with 'for' loop.
> - Fix indent of 'if' statement.
>
> Signed-off-by: Tetsuya Mukawa
> ---
> lib/librte_eal/common/include/rte_pci.h | 2 +
> lib/librte_ether/rte_ethdev.c | 454
> +---
> lib/librte_ether/rte_ethdev.h | 5 +
> 3 files changed, 186 insertions(+), 275 deletions(-)
>
> diff --git a/lib/librte_eal/common/include/rte_pci.h
> b/lib/librte_eal/common/include/rte_pci.h
> index 7b48b55..7f2d699 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -207,6 +207,8 @@ struct rte_pci_driver {
> #define RTE_PCI_DRV_FORCE_UNBIND 0x0004
> /** Device driver supports link state interrupt */
> #define RTE_PCI_DRV_INTR_LSC 0x0008
> +/** Device driver supports detaching capability */
> +#define RTE_PCI_DRV_DETACHABLE 0x0010
>
> /**< Internal use only - Macro used by pci addr parsing functions **/
> #define GET_PCIADDR_FIELD(in, fd, lim, dlm) \
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index ea3a1fb..a79fa5b 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -175,6 +175,16 @@ enum {
> STAT_QMAP_RX
> };
>
> +enum {
> + DEV_INVALID = 0,
> + DEV_VALID,
> +};
> +
> +enum {
> + DEV_DISCONNECTED = 0,
> + DEV_CONNECTED
> +};
The commit log explains what is an attached port but not what means
valid or connected.
These enums should have a comment to explain their usage.
> +
> static inline void
> rte_eth_dev_data_alloc(void)
> {
> @@ -201,19 +211,34 @@ rte_eth_dev_allocated(const char *name)
> {
> unsigned i;
>
> - for (i = 0; i < nb_ports; i++) {
> - if (strcmp(rte_eth_devices[i].data->name, name) == 0)
> + for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> + if ((rte_eth_devices[i].attached == DEV_CONNECTED) &&
> + strcmp(rte_eth_devices[i].data->name, name) == 0)
> return _eth_devices[i];
> }
> return NULL;
> }
>
> +static uint8_t
> +rte_eth_dev_allocate_new_port(void)
> +{
> + unsigned i;
> +
> + for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> + if (rte_eth_devices[i].attached == DEV_DISCONNECTED)
> + return i;
> + }
> + return RTE_MAX_ETHPORTS;
> +}
This function does not allocate a new port.
It get the first free port id.
Is uint8_t sill a good size for hotpluggable virtual device ids?
> +
> struct rte_eth_dev *
> rte_eth_dev_allocate(const char *name)
> {
> + uint8_t port_id;
> struct rte_eth_dev *eth_dev;
>
> - if (nb_ports == RTE_MAX_ETHPORTS) {
> + port_id = rte_eth_dev_allocate_new_port();
> + if (port_id == RTE_MAX_ETHPORTS) {
> PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports\n");
> return NULL;
> }
> @@ -226,10 +251,12 @@ rte_eth_dev_allocate(const char *name)
> return NULL;
> }
>
> - eth_dev = _eth_devices[nb_ports];
> - eth_dev->data = _eth_dev_data[nb_ports];
> + eth_dev = _eth_devices[port_id];
> + eth_dev->data = _eth_dev_data[port_id];
> snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
> - eth_dev->data->port_id = nb_ports++;
> + eth_dev->data->port_id = port_id;
> + eth_dev->attached = DEV_CONNECTED;
> + nb_ports++;
> return eth_dev;
> }
>
> @@ -283,6 +310,7 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
> (unsigned) pci_dev->id.device_id);
> if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> rte_free(eth_dev->data->dev_private);
> + eth_dev->attached = DEV_DISCONNECTED;
> nb_ports--;
> return diag;
> }
> @@ -308,10 +336,28 @@ rte_eth_driver_register(struct eth_driver *eth_drv)
> rte_eal_pci_register(_drv->pci_drv);
> }
>
> +enum {
> + NO_TRACE = 0,
> + TRACE
> +};
What means this enum?
> +
> +static int
> +rte_eth_dev_validate_port(uint8_t port_id, int trace)
> +{
> + if (port_id >= RTE_MAX_ETHPORTS ||
> + rte_eth_devices[port_id].attached != DEV_CONNECTED) {
> + if (trace) {
> + PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> + }
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
> -Original Message- > From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp] > Sent: Monday, February 16, 2015 4:14 AM > To: dev at dpdk.org > Cc: Qiu, Michael; Iremonger, Bernard; Tetsuya Mukawa > Subject: [PATCH v8 03/14] eal/pci,ethdev: Remove assumption that port will > not be detached > > To remove assumption, do like followings. > > This patch adds "RTE_PCI_DRV_DETACHABLE" to drv_flags of rte_pci_driver > structure. The flags > indicate the driver can detach devices at runtime. > Also, remove assumption that port will not be detached. > > To remove the assumption. > - Add 'attached' member to rte_eth_dev structure. > This member is used for indicating the port is attached, or not. > - Add rte_eth_dev_allocate_new_port(). > This function is used for allocating new port. > > v8: > - NONE_TRACE is changed to NO_TRACE. > (Thanks to Iremonger, Bernard) > v5: > - Change parameters of rte_eth_dev_validate_port() to cleanup code. > v4: > - Use braces with 'for' loop. > - Fix indent of 'if' statement. > > Signed-off-by: Tetsuya Mukawa Acked-by: Bernard Iremonger
[dpdk-dev] [PATCH v8 03/14] eal/pci, ethdev: Remove assumption that port will not be detached
To remove assumption, do like followings.
This patch adds "RTE_PCI_DRV_DETACHABLE" to drv_flags of rte_pci_driver
structure. The flags indicate the driver can detach devices at runtime.
Also, remove assumption that port will not be detached.
To remove the assumption.
- Add 'attached' member to rte_eth_dev structure.
This member is used for indicating the port is attached, or not.
- Add rte_eth_dev_allocate_new_port().
This function is used for allocating new port.
v8:
- NONE_TRACE is changed to NO_TRACE.
(Thanks to Iremonger, Bernard)
v5:
- Change parameters of rte_eth_dev_validate_port() to cleanup code.
v4:
- Use braces with 'for' loop.
- Fix indent of 'if' statement.
Signed-off-by: Tetsuya Mukawa
---
lib/librte_eal/common/include/rte_pci.h | 2 +
lib/librte_ether/rte_ethdev.c | 454 +---
lib/librte_ether/rte_ethdev.h | 5 +
3 files changed, 186 insertions(+), 275 deletions(-)
diff --git a/lib/librte_eal/common/include/rte_pci.h
b/lib/librte_eal/common/include/rte_pci.h
index 7b48b55..7f2d699 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -207,6 +207,8 @@ struct rte_pci_driver {
#define RTE_PCI_DRV_FORCE_UNBIND 0x0004
/** Device driver supports link state interrupt */
#define RTE_PCI_DRV_INTR_LSC 0x0008
+/** Device driver supports detaching capability */
+#define RTE_PCI_DRV_DETACHABLE 0x0010
/**< Internal use only - Macro used by pci addr parsing functions **/
#define GET_PCIADDR_FIELD(in, fd, lim, dlm) \
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index ea3a1fb..a79fa5b 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -175,6 +175,16 @@ enum {
STAT_QMAP_RX
};
+enum {
+ DEV_INVALID = 0,
+ DEV_VALID,
+};
+
+enum {
+ DEV_DISCONNECTED = 0,
+ DEV_CONNECTED
+};
+
static inline void
rte_eth_dev_data_alloc(void)
{
@@ -201,19 +211,34 @@ rte_eth_dev_allocated(const char *name)
{
unsigned i;
- for (i = 0; i < nb_ports; i++) {
- if (strcmp(rte_eth_devices[i].data->name, name) == 0)
+ for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+ if ((rte_eth_devices[i].attached == DEV_CONNECTED) &&
+ strcmp(rte_eth_devices[i].data->name, name) == 0)
return _eth_devices[i];
}
return NULL;
}
+static uint8_t
+rte_eth_dev_allocate_new_port(void)
+{
+ unsigned i;
+
+ for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+ if (rte_eth_devices[i].attached == DEV_DISCONNECTED)
+ return i;
+ }
+ return RTE_MAX_ETHPORTS;
+}
+
struct rte_eth_dev *
rte_eth_dev_allocate(const char *name)
{
+ uint8_t port_id;
struct rte_eth_dev *eth_dev;
- if (nb_ports == RTE_MAX_ETHPORTS) {
+ port_id = rte_eth_dev_allocate_new_port();
+ if (port_id == RTE_MAX_ETHPORTS) {
PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports\n");
return NULL;
}
@@ -226,10 +251,12 @@ rte_eth_dev_allocate(const char *name)
return NULL;
}
- eth_dev = _eth_devices[nb_ports];
- eth_dev->data = _eth_dev_data[nb_ports];
+ eth_dev = _eth_devices[port_id];
+ eth_dev->data = _eth_dev_data[port_id];
snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
- eth_dev->data->port_id = nb_ports++;
+ eth_dev->data->port_id = port_id;
+ eth_dev->attached = DEV_CONNECTED;
+ nb_ports++;
return eth_dev;
}
@@ -283,6 +310,7 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
(unsigned) pci_dev->id.device_id);
if (rte_eal_process_type() == RTE_PROC_PRIMARY)
rte_free(eth_dev->data->dev_private);
+ eth_dev->attached = DEV_DISCONNECTED;
nb_ports--;
return diag;
}
@@ -308,10 +336,28 @@ rte_eth_driver_register(struct eth_driver *eth_drv)
rte_eal_pci_register(_drv->pci_drv);
}
+enum {
+ NO_TRACE = 0,
+ TRACE
+};
+
+static int
+rte_eth_dev_validate_port(uint8_t port_id, int trace)
+{
+ if (port_id >= RTE_MAX_ETHPORTS ||
+ rte_eth_devices[port_id].attached != DEV_CONNECTED) {
+ if (trace) {
+ PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+ }
+ return DEV_INVALID;
+ } else
+ return DEV_VALID;
+}
+
int
rte_eth_dev_socket_id(uint8_t port_id)
{
- if (port_id >= nb_ports)
+ if (rte_eth_dev_validate_port(port_id, NO_TRACE) == DEV_INVALID)
return -1;
return rte_eth_devices[port_id].pci_dev->numa_node;
}
@@ -369,10 +415,8 @@ rte_eth_dev_rx_queue_start(uint8_t port_id, uint16_t
rx_queue_id)
* in a multi-process setup*/
PROC_PRIMARY_OR_ERR_RET(-E_RTE_SECONDARY);
- if (port_id >=
