Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1741777 > unrolled thread

Re: [PATCH RFC 3/5] Add KSZ8795 switch driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 3/5] Add KSZ8795 switch driver Pavel Machek <pavel@ucw.cz> - 2017-09-28 20:50 +0200
    Re: [PATCH RFC 3/5] Add KSZ8795 switch driver Florian Fainelli <f.fainelli@gmail.com> - 2017-09-28 20:50 +0200
    RE: [PATCH RFC 3/5] Add KSZ8795 switch driver <Tristram.Ha@microchip.com> - 2017-09-29 21:00 +0200

#1741777 — Re: [PATCH RFC 3/5] Add KSZ8795 switch driver

[Multipart message — attachments visible in raw view] — view raw

Hi!

On Mon 2017-09-18 20:27:13, Tristram.Ha@microchip.com wrote:
> > > +/**
> > > + * Some counters do not need to be read too often because they are less
> > likely
> > > + * to increase much.
> > > + */
> > 
> > What does comment mean? Are you caching statistics, and updating
> > different values at different rates?
> > 
> 
> There are 34 counters.  In normal case using generic bus I/O or PCI to read them
> is very quick, but the switch is mostly accessed using SPI, or even I2C.  As the SPI
> access is very slow and cannot run in interrupt context I keep worrying reading
> the MIB counters in a loop for 5 or more ports will prevent other critical hardware
> access from executing soon enough.  These accesses can be getting 1588 PTP
> timestamps and opening/closing ports.  (RSTP Conformance Test sends test traffic
> to port supposed to be closed/opened after receiving specific RSTP
> BPDU.)

Hmm. Ok, interesting.

I wonder how well this is going to work if userspace actively 'does
something' with the switch.

It seems to me that even if your statistics code is careful not to do
'a lot' of accesses at the same time, userspace can use other parts of
the driver to do the same, and thus cause same unwanted effects...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1741778

On 09/28/2017 11:40 AM, Pavel Machek wrote:
> Hi!
> 
> On Mon 2017-09-18 20:27:13, Tristram.Ha@microchip.com wrote:
>>>> +/**
>>>> + * Some counters do not need to be read too often because they are less
>>> likely
>>>> + * to increase much.
>>>> + */
>>>
>>> What does comment mean? Are you caching statistics, and updating
>>> different values at different rates?
>>>
>>
>> There are 34 counters.  In normal case using generic bus I/O or PCI to read them
>> is very quick, but the switch is mostly accessed using SPI, or even I2C.  As the SPI
>> access is very slow and cannot run in interrupt context I keep worrying reading
>> the MIB counters in a loop for 5 or more ports will prevent other critical hardware
>> access from executing soon enough.  These accesses can be getting 1588 PTP
>> timestamps and opening/closing ports.  (RSTP Conformance Test sends test traffic
>> to port supposed to be closed/opened after receiving specific RSTP
>> BPDU.)
> 
> Hmm. Ok, interesting.
> 
> I wonder how well this is going to work if userspace actively 'does
> something' with the switch.
> 
> It seems to me that even if your statistics code is careful not to do
> 'a lot' of accesses at the same time, userspace can use other parts of
> the driver to do the same, and thus cause same unwanted effects...

A few switches have a MIB snapshot feature that is implemented such that
accessing the snapshot does not hog the remainder of the switch
registers, is this something possible on KSZ switches?

Tangential: net-next is currently open, so now would be a good time to
send a revised version of your patch series to target possibly 4.15 with
an initial implementation. Please fix the cover-letter and patch
threading such that they look like the following:

[PATCH 0/X]
   [PATCH 1/X]
   [PATCH 2/X]
   etc..

Right now this shows up as separate emails/patches and this is very
annoying to follow as a thread.

Thank you
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1742290

> On Mon 2017-09-18 20:27:13, Tristram.Ha@microchip.com wrote:
> > > > +/**
> > > > + * Some counters do not need to be read too often because they are
> less
> > > likely
> > > > + * to increase much.
> > > > + */
> > >
> > > What does comment mean? Are you caching statistics, and updating
> > > different values at different rates?
> > >
> >
> > There are 34 counters.  In normal case using generic bus I/O or PCI to read
> them
> > is very quick, but the switch is mostly accessed using SPI, or even I2C.  As
> the SPI
> > access is very slow and cannot run in interrupt context I keep worrying
> reading
> > the MIB counters in a loop for 5 or more ports will prevent other critical
> hardware
> > access from executing soon enough.  These accesses can be getting 1588
> PTP
> > timestamps and opening/closing ports.  (RSTP Conformance Test sends test
> traffic
> > to port supposed to be closed/opened after receiving specific RSTP
> > BPDU.)
> 
> Hmm. Ok, interesting.
> 
> I wonder how well this is going to work if userspace actively 'does
> something' with the switch.
> 
> It seems to me that even if your statistics code is careful not to do
> 'a lot' of accesses at the same time, userspace can use other parts of
> the driver to do the same, and thus cause same unwanted effects...

If the user calls "ethtool -S" in a tight loop the system will waste a lot of
CPU time, but this is more like a user error.
Another solution is not to schedule to read the MIB counters in that
function call.  I think I was doing a favor to update the MIB counters
sooner as the user probably wants to find out what is wrong with the
switch by reading the MIB counters and checking them several times.
For system tracking like SNMP I think it is likely a separate mechanism
is used to gather those information.  If I am wrong that function definitely
needs to be modified.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web