| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <575FF79D.6030603@linaro.org> Date: Tue, 14 Jun 2016 15:25:01 +0300 From: Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org> To: Schuyler Patton <spatton@...com>, mugunthanvnm@...com, linux-kernel@...r.kernel.org Cc: grygorii.strashko@...com, linux-omap@...r.kernel.org, netdev@...r.kernel.org, robh+dt@...nel.org, pawel.moll@....com, mark.rutland@....com, ijc+devicetree@...lion.org.uk, galak@...eaurora.org, bcousson@...libre.com, tony@...mide.com, devicetree@...r.kernel.org Subject: Re: [PATCH v2 0/2] net: ethernet: ti: cpsw: delete rx_descs property On 11.06.16 02:04, Schuyler Patton wrote: > > > On 06/08/2016 07:03 PM, Ivan Khoronzhuk wrote: >> >> >> On 09.06.16 02:11, Schuyler Patton wrote: >>> >>> >>> On 06/08/2016 09:06 AM, Ivan Khoronzhuk wrote: >>>> >>>> >>>> On 08.06.16 17:01, Ivan Khoronzhuk wrote: >>>>> Hi Schuyer, >>>>> >>>>> On 07.06.16 18:26, Schuyler Patton wrote: >>>>>> Hi, >>>>>> >>>>>> On 06/07/2016 08:59 AM, Ivan Khoronzhuk wrote: >>>>>>> There is no reason in rx_descs property because davinici_cpdma >>>>>>> driver splits pool of descriptors equally between tx and rx channels. >>>>>>> So, this patch series makes driver to use available number of >>>>>>> descriptors for rx channels. >>>>>> >>>>>> I agree with the idea of consolidating how the descriptors are defined because of >>>>>> the two variable components, number and size of the pool can be confusing to >>>>>> end users. I would like to request to change how it is being proposed here. >>>>>> >>>>>> I think the number of descriptors should be left in the device tree source file as >>>>>> is and remove the BD size variable and have the driver calculate the size of the >>>>>> pool necessary to support the descriptor request. From an user perspective it is >>>>>> easier I think to be able to list the number of descriptors necessary vs. the size >>>>>> of the pool. >>>>>> >>>>>> Since the patch series points out how it is used so in the driver so to make that >>>>>> consistent is perhaps change rx_descs to total_descs. >>>>>> >>>>>> Regards, >>>>>> Schuyler >>>>> >>>>> The DT entry for cpsw doesn't have property for size of the pool. >>>>> It contains only BD ram size, if you mean this. The size of the pool is >>>>> software decision. Current version of DT entry contain only rx desc number. >>>>> That is not correct, as it depends on the size of the descriptor, which is also >>>>> h/w parameter. The DT entry has to describe only h/w part and shouldn't contain >>>>> driver implementation details, and I'm looking on it from this perspective. >>>>> >>>>> Besides, rx_descs describes only rx number of descriptors, that are taken from >>>>> the same pool as tx descriptors, and setting rx desc to some new value doesn't >>>>> mean that rest of them are freed for tx. Also, I'm going to send series that >>>>> adds multi channel support to the driver, and in this case, splitting of the >>>>> pool will be more sophisticated than now, after what setting those parameters >>>>> for user (he should do this via device tree) can be even more confusing. But, >>>> should -> shouldn't >>>> >>>>> as it's supposed, it's software decision that shouldn't leak to the DT. >>> >>> If this rx-desc field is removed how will the number of descriptors be set? >>> >>> This field has been used to increase the number of descriptors so high >>> volume short packets are not dropped due to descriptor exhaustion. The current >>> default number of 64 rx descriptors is too low for gigabit networks. Some users >>> have a strong requirement for zero loss of UDP packets setting this field to a >>> larger number and setting the descriptors off-chip was a means to solve >>> the problem. >> The current implementation of cpdma driver splits descs num on 2 parts equally. >> Total number = 256, then 128 reserved for rx and 128 for tx, but setting this to >> 64, simply limits usage of reserved rx descriptors to 64, so that: >> 64 rx descs, 128 tx descs and 64 are always present in the pool but cannot be used, >> (as new rx descriptor is allocated only after previous was freed). >> That means, 64 rx descs are unused. In case of rx descriptor exhaustion, an user can >> set rx_descs to 128, for instance, in this case all descriptors will be in use, but then question, >> why intentionally limit number of rx descs, anyway rest 64 descs cannot be used for other >> purposes. In case of this patch, all rx descs are in use, and no need to correct number >> of rx descs anymore, use all of them....and it doesn't have impact on performance, as >> anyway, bunch of rx descs were simply limited by DT and unused. So, probably, there is no >> reason to worry about that. > > When we see this issue we set the descriptors to DDR and put a large number > in the desc count. unfortunately I wish I could provide a number, usually the issue > is a volume burst of short UDP packets. This patch sets this rx_descs to maximum possible, after what no need to set it in DT. From user point of view, no need to play with rx_descs, it's done already, with current version of driver. All is still the same, like rx_descs was set in DT to >= 128 (rx_descs didn't work as expected). With DDR there is also no issues also. The cpsw takes as much descs for rx as it's allowed by cpdma. > >> >> PS: >> It doesn't concern this patch, but, which PPS makes rx descs to be exhausted?... >> (In this case "desc_alloc_fail" counter contains some value for rx channel, >> and can be read with "ethtool -S eth0". Also, the user will be WARNed ON by the driver) >> >> it's interesting to test it, I'm worrying about, because in case of multichannel, >> the pool is split between all channels... they are throughput limited, but >> anyway, it's good to correlate the number of descs with throughput assigned to >> a channel, if possible. That has to be possible, if setting to 128 helps, then >> has to be value between 64 and 128 to make handling of rx packets fast enough. >> After what, can be calculated correlation between number of rx descs and throughput >> split between channels.... > > With gigabit networks 64 or 128 rx descriptors is not going to enough to fix the > DMA overrun problem. Usually we set this number to an arbitrarily large 2000 > descriptors in external DDR to demonstrate it is possible to not drop packets. All > this does is move the problem higher up so that the drops occur in network > stack if the ARM is overloaded. With the high speed networks I would like > to propose that the descriptor pool or pools are moved to DDR by default. It would > be nice to have some reconfigurability or set a pool size that reduces or eliminates > the DMA issue that is seen in these types of applications. > > This test gets used a lot, which is to send very short UDP packets. If I have the math > right, a 52 byte (64 byte with the inter-frame gap) UDP packet the default 64 > descriptors gets consumed in roughly 33uS. There are the switch fifos which will also > allow some headroom, but a user was dropping packets at the switch when they > were bursting 360 packets at the processor on a gigabit link > >> >>> >>>>> >>>>> >>>>>>> >>>>>>> Based on master branch >>>>>>> >>>>>>> Since v1: >>>>>>> - separate device tree and driver patches >>>>>>> - return number of rx buffers from cpdma driver >>>>>>> >>>>>>> Ivan Khoronzhuk (2): >>>>>>> net: ethernet: ti: cpsw: remove rx_descs property >>>>>>> Documentation: DT: cpsw: remove rx_descs property >>>>>>> >>>>>>> Documentation/devicetree/bindings/net/cpsw.txt | 1 - >>>>>>> arch/arm/boot/dts/am33xx.dtsi | 1 - >>>>>>> arch/arm/boot/dts/am4372.dtsi | 1 - >>>>>>> arch/arm/boot/dts/dm814x.dtsi | 1 - >>>>>>> arch/arm/boot/dts/dra7.dtsi | 1 - >>>>>>> drivers/net/ethernet/ti/cpsw.c | 13 +++---------- >>>>>>> drivers/net/ethernet/ti/cpsw.h | 1 - >>>>>>> drivers/net/ethernet/ti/davinci_cpdma.c | 6 ++++++ >>>>>>> drivers/net/ethernet/ti/davinci_cpdma.h | 1 + >>>>>>> 9 files changed, 10 insertions(+), 16 deletions(-) >>>>>>> >>>>>> >>>>> >>>> >>> >> > -- Regards, Ivan Khoronzhuk
Powered by blists - more mailing lists