Message-ID: <CAKgT0Ud6rUp5c1+fsCpnN9YW=b5g2Fd5pejGTj3kZOgvfskfMw@mail.gmail.com>
Date:   Wed, 24 Jan 2018 08:01:41 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Benjamin Poirier <benjamin.poirier@...il.com>
Cc:     Shrikrishna Khare <skhare@...are.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        Netdev <netdev@...r.kernel.org>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [Intel-wired-lan] [RFC PATCH] e1000e: Remove Other from EIAC.

On Wed, Jan 24, 2018 at 12:35 AM, Benjamin Poirier
<benjamin.poirier@...il.com> wrote:
> On 2018/01/22 10:01, Alexander Duyck wrote:
> [...]
>> >
>> > If the patch that I submitted for the current vmware issue is merged,
>> > the significant commits that are left are:
>> >
>> > 0a8047ac68e5 e1000e: Fix msi-x interrupt automask (v4.5-rc1)
>> >         Fixes a problem in the irq disabling of the napi implementation.
>>
>> This one I fully agree is still needed.
>>
>> > 19110cfbb34d e1000e: Separate signaling for link check/link up
>> >              (v4.15-rc1)
>> >         Fixes link flapping caused by a race condition in link
>> >         detection. It was found because the Other interrupt was being
>> >         triggered sort of spuriously by rxo.
>>
>> This one is somewhat iffy. I am not sure if the patch description
>> really matches what it is doing. It doesn't appear to do what it says
>> it is trying to do since clearing get_link_status will still trigger a
>> link up, it just takes an extra 2 seconds. I think there may be issues
>> if you aren't using autoneg, as I don't see how you are getting the
>> link to report up other than the fact that mac->get_link_status has
>> been cleared but we are reporting a pseduo-error. In addition it is
>> only really needed after the RXO problem was introduced which really
>> didn't exist until after we stopped checking for LSC. One interesting
>> test we may want to look at is to see if there is an additional delay
>> in a link coming up for a non-autoneg setup. If we find an additional
>> 2 second delay then I would be even more confident that this patch has
>> a bug.
>
> It seems like you're right but I didn't look into this part of the problem in
> detail yet. I'll get back to it.
>
>>
>> > 4aea7a5c5e94 e1000e: Avoid receiver overrun interrupt bursts (v4.15-rc1)
>> >         Fixes Other interrupt bursts during sustained rxo conditions.
>>
>> So the RXO problem probably didn't exist until we stopped checking for
>> the OTHER and LSC bits in the "other" interrupt handler. Yes there
>> would be more "other" cause interrupts, but they shouldn't have been
>> causing much in the way of issues since the get_link_status value
>> never changed. Personally I would lean more toward the option of
>
> I agree. I tested rxo behavior on commit 4d432f67ff00 ("e1000e: Remove
> unreachable code", v4.5-rc1) which is before any significant change in that
> area. (I force rxo by adding mdelay(10) to e1000_clean_rx_irq and sending a
> netperf UDP_STREAM from another host). In case of sustained rxo condition, we
> get repeated Other interrupts. Handling these irqs is useless work that could
> be avoided when the system is already overloaded but it doesn't lead to
> misbehavior like the race condition described in the log of commit
> 19110cfbb34d ("e1000e: Separate signaling for link check/link up", v4.15-rc1).
>
> However, I noticed something unexpected. It seems like reading ICR doesn't
> clear every bit that's set in IAM, most notably not rxo. In a different test,
> I was doing a single write of RXO | OTHER to ICS, then two subsequent reads of
> icr gave 0x01000041. OTOH, writing a bit to ICS reliably clears it. So if you
> want to remove RXO interrupt mitigation, you should at least add a write of
> RXO to ICR, to clear it. On my system it reduced Other interrupts from
> ~17000/s to ~1700/s when using the mdelay testing approach.

That makes sense. I would say we should probably just put together a
mask of all possible causes for "other" interrupts that we don't
actually care about and just clear them explicitly when we clear the
"other" bit.

>> reverting this patch and instead just focus on testing OTHER and LSC
>> as we originally were so that we don't risk messing up NAPI by messing
>> with ring state from a non-ring interrupt.
>>
>> I will try to get to these later this week if you would like.
>> Unfortunately I don't have any of these devices in any of my
>> development systems so I have to go chase one down. Otherwise you are
>> free to take these on and tell me if I have made another flawed
>> assumption somewhere, but I am thinking the RXO issue goes away if we
>> get the original "other" interrupt routine back to where it was.
>>
>> So the last bit in all this ends up being that because of 0a8047ac68e5
>> e1000e: Fix msi-x interrupt automask (v4.5-rc1) we don't seem to
>> auto-clear interrupt causes anymore on ICR read. I am not certain what
>> the impact of this is. I would be interested in finding out if a cause
>> left set will trigger an interrupt storm or if it just goes quiet when
>> we just leave the value high. If it goes quiet then that in itself
>> might solve the RXO interrupt burst problem if we don't clear it.
>> Otherwise we need to make certain to clear all of the causes that can
>> trigger the "other" interrupt to fire regardless of if we service the
>> events or not.
>
> In MSI-X mode, as long as Other is not set in ICR, nothing will happen even if
> the bits related to Other (LSC, RXO, MDAC, SRPD, ACK, MNG) are set. However,
> that doesn't solve the rxo interrupt burst because an rxo condition in
> hardware sets both RXO and Other, so it triggers an interrupt.

Right. I expect we will still have more interrupts. But by just
clearing it and moving on we at least resolve the issue without
introducing possible new ones.

I would say we could probably clear everything that is on that list
that is not LSC explicitly since we really don't care about those
events. If that changes then in the future we could avoid clearing
them until we capture an actual event.

Thanks.

- Alex

Powered by blists - more mailing lists