From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-00191d01.pphosted.com (mx0b-00191d01.pphosted.com [67.231.157.136]) by dpdk.org (Postfix) with ESMTP id E54D7324D for ; Fri, 12 Jan 2018 10:38:31 +0100 (CET) Received: from pps.filterd (m0083689.ppops.net [127.0.0.1]) by m0083689.ppops.net-00191d01. (8.16.0.21/8.16.0.21) with SMTP id w0C9Ymio004702; Fri, 12 Jan 2018 04:38:31 -0500 Received: from alpi155.enaf.aldc.att.com (sbcsmtp7.sbc.com [144.160.229.24]) by m0083689.ppops.net-00191d01. with ESMTP id 2feks2dxfm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 12 Jan 2018 04:38:31 -0500 Received: from enaf.aldc.att.com (localhost [127.0.0.1]) by alpi155.enaf.aldc.att.com (8.14.5/8.14.5) with ESMTP id w0C9cUb5028887; Fri, 12 Jan 2018 04:38:30 -0500 Received: from mlpi407.sfdc.sbc.com (mlpi407.sfdc.sbc.com [130.9.128.239]) by alpi155.enaf.aldc.att.com (8.14.5/8.14.5) with ESMTP id w0C9cMBh028859 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 12 Jan 2018 04:38:23 -0500 Received: from zlp27128.vci.att.com (zlp27128.vci.att.com [135.66.87.50]) by mlpi407.sfdc.sbc.com (RSA Interceptor); Fri, 12 Jan 2018 09:38:08 GMT Received: from zlp27128.vci.att.com (zlp27128.vci.att.com [127.0.0.1]) by zlp27128.vci.att.com (Service) with ESMTP id F0B4E4000417; Fri, 12 Jan 2018 09:38:07 +0000 (GMT) Received: from gbcdccas01.intl.att.com (unknown [135.76.180.9]) by zlp27128.vci.att.com (Service) with ESMTPS id 9C30840006B6; Fri, 12 Jan 2018 09:38:07 +0000 (GMT) Received: from GBCDCMBX03.intl.att.com ([135.76.31.134]) by gbcdccas01.intl.att.com ([135.76.180.9]) with mapi id 14.03.0361.001; Fri, 12 Jan 2018 09:38:05 +0000 From: "Dewar, Alan" To: "'Dumitrescu, Cristian'" , "'alangordondewar@gmail.com'" CC: "'dev@dpdk.org'" , "'Alan Dewar'" , "'Kantecki, Tomasz'" , "'Singh, Jasvinder'" Thread-Topic: [PATCH v5] sched: make RED scaling configurable Thread-Index: AQHTiJU2egTnYjLTxUqJVRrQuGiHVKNuqhuAgAFKeIA= Date: Fri, 12 Jan 2018 09:38:04 +0000 Message-ID: <3F9268EEC0E43747A5FFFC6B48EF0321FC0CAF@gbcdcmbx03.intl.att.com> References: <1507022514-21831-1> <1515425232-18888-1-git-send-email-alan.dewar@att.com> <3EB4FA525960D640B5BDFFD6A3D891267BAFB4EE@IRSMSX108.ger.corp.intel.com> In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D891267BAFB4EE@IRSMSX108.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [135.160.174.85] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-RSA-Inspected: yes X-RSA-Classifications: public X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2018-01-12_05:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_policy_notspam policy=outbound_policy score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1801120130 X-Mailman-Approved-At: Sun, 14 Jan 2018 18:13:42 +0100 Subject: Re: [dpdk-dev] [PATCH v5] sched: make RED scaling configurable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Jan 2018 09:38:32 -0000 Hi Cristian, Responses inline below. Regards Alan > -----Original Message----- > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]=20 > Sent: Thursday, January 11, 2018 1:11 PM > To: alangordondewar@gmail.com > Cc: dev@dpdk.org; Alan Dewar ; Kantecki, Tomasz ; Singh, Jasvinder > Subject: RE: [PATCH v5] sched: make RED scaling configurable > > Hi Alan, > > More issues and questions below: > >... > > > diff --git a/lib/librte_sched/rte_red.c b/lib/librte_sched/rte_red.h > I suggest we use uint32_t for rte_red_scaling and rte_red_max_threshold, = even though their values can fit into uint8_t and uint16_t respectively. Will do. > > diff --git a/lib/librte_sched/rte_red.c b/lib/librte_sched/rte_red.c > > +int > > +rte_red_set_scaling(uint16_t max_red_queue_length) { > > + int8_t count; > > + > > + if (rte_red_init_done) > > + /** > > + * Can't change the scaling once the red table has been > > + * computed. > > + */ > > + return -1; > > Is there a reason why we cannot simply reset the scaling here? Actually we could, but I was originally thinking that you might be happier = keeping with a one-time RED initialization function, but then had to introd= uce the rte_reset_red_scaling function for the unit-tests. I'm happy to do = RED reinitialization here, if you are. > > + > > + if (max_red_queue_length < RTE_RED_MIN_QUEUE_LENGTH) > > + return -2; > > + > > + if (max_red_queue_length > RTE_RED_MAX_QUEUE_LENGTH) > > + return -3; > > + > > + if (!rte_is_power_of_2(max_red_queue_length)) > > + return -4; > > + > > + count =3D 0; > > + while (max_red_queue_length !=3D 0) { > > + max_red_queue_length >>=3D 1; > > + count++; > > + } > > This does not look right to me. I think you want to compute the log2 of m= ax_red_queue_length here, but your result (count) is bigger by 1 than it sh= ould be, right? > When max_red_queue_length =3D RTE_RED_DEFAULT_QUEUE_LENGTH =3D 1024, the = result should be: count =3D RTE_RED_SCALING =3D 10, not 11. > Well spotted. > I suggest you use rte_bsf32() function for this purpose. I wasn't aware of this function, it appears to do the necessary so will do. > > + > > + rte_red_scaling -=3D count - RTE_RED_SCALING; > > Why not simply: rte_red_scaling =3D count? Stupidity on my part! - Will do. > > + rte_red_max_threshold =3D max_red_queue_length - 1; > > + return 0; > > +} > > + > > > diff --git a/lib/librte_sched/rte_sched_version.map > > b/lib/librte_sched/rte_sched_version.map > > index 3aa159a..262bece 100644 > > --- a/lib/librte_sched/rte_sched_version.map > > +++ b/lib/librte_sched/rte_sched_version.map > > @@ -29,3 +29,10 @@ DPDK_2.1 { > > rte_sched_port_pkt_read_color; > >=20 > > } DPDK_2.0; > > + > > +DPDK_17.08 { > > + global; > > + > > + __rte_red_reset; > > + rte_red_set_scaling; > > +} DPDK_2.1; > > You need to put the correct DPDK release number here. Will do. > You also need to do more work to make sure the share library support is = not broken: > -need to increase LIBABIVER in Makefile > -need to update the library .so number in release notes -need to check th= at build does not fail for shared libraries > > (You can use this patch as example: https://urldefense.proofpoint.com/v2/= url?u=3Dhttps-3A__dpdk.org_dev_patchwork_patch_33109_&d=3DDwIFAg&c=3DLFYZ-o= 9_HUMeMTSQicvjIg&r=3DMSobi4bsl2plY_3b1jvY6kXt-SHzSxMj5YhuyxfTjJE&m=3DD7OTgD= yX27pcg-xs8N7FQzX4lAkkcPqbPAhJp_IyrNI&s=3DmCvsqitq8Y6vD3UC-r1WEGtrRp17bsmJa= EHGby97zYw&e=3D ) Okay, I need to look into this stuff - I'll come back with some questions i= f anything isn't clear. > > diff --git a/test/test/test_red.c b/test/test/test_red.c > The RED test code fails to link, please fix: Yes - will do. > export RTE_TARGET=3Dx86_64-native-linuxapp-gcc > > [ ]$ ~/git_dpdk_red/dpdk$ make -C test/test > make: Entering directory 'dpdk/test/test' >... > LD test > test_red.o: In function `test_rte_red_init': > test_red.c:(.text+0x2cd): undefined reference to `__rte_red_reset' > test_red.c:(.text+0x2da): undefined reference to `rte_red_set_scaling' > test_red.c:(.text+0x2ed): undefined reference to `rte_red_max_threshold' > test_red.o: In function `increase_actual_qsize': > test_red.c:(.text+0x48c): undefined reference to `rte_red_scaling' > test_red.c:(.text+0x533): undefined reference to `rte_red_scaling' > test_red.o: In function `enqueue_dequeue_func': > test_red.c:(.text+0x70c): undefined reference to `rte_red_scaling' > test_red.c:(.text+0x7a5): undefined reference to `rte_red_scaling' > test_red.o: In function `increase_average_qsize': > test_red.c:(.text+0x95c): undefined reference to `rte_red_scaling' > test_red.o:test_red.c:(.text+0x9f1): more undefined references to `rte_re= d_scaling' follow > collect2: error: ld returned 1 exit status > dpdk/mk/rte.app.mk:306: recipe for target 'test' failed > make: *** [test] Error 1 > make: Leaving directory 'dpdk/test/test' > > Regards, > Cristian