On Mon, 2022-05-09 at 14:44 +0530, Harini Katakam wrote:

Hi Robert,

Thanks for the patch.

<snip>

quoted

Originally I thought there might be a correctness issue with calling it
unconditionally, but looking at it further I don't think there really is.
The
FreeBSD driver for this hardware also seems to always do the TX restart in
the
interrupt handler if there are packets in the TX queue.

I think the only real issue is whether it's better performance wise to do
it
all the time rather than only after the hardware asserts a TXUBR interrupt.
I
expect it would be worse to do it all the time, as that would mean an extra
MMIO read, spinlock, MMIO read and MMIO write, versus just a read barrier
and
checking a flag in memory.

I agree that doing TX restart only on UBR is better.

quoted

quoted

But should there anyways be some condition for the tx side handling, as
I suppose macb_poll() runs when there is rx interrupt even if tx side
has nothing to process?

I opted not to do that for this case, as it should be pretty harmless and
cheap
to just check the TX ring to see if any packets have been completed yet,
rather
than actually tracking if a TX completion was pending. That seems to be the
standard practice in some other drivers (r8169, etc.)

In this implementation the TX interrupt bit is being cleared and TX
processing is
scheduled when there is an RX interrupt as well as vice versa. Could you
please
consider using the check "status & MACB_BIT(TCOMP)" for TX interrupt and
NAPI?
If I understand your reply above right, you mention that the above check is
more
expensive than parsing the TX ring for new data. In unbalanced traffic
scenarios i.e.
server only or client only, will this be efficient?

In the v2 of this patch set I'm about to submit, I'm splitting the NAPI
structures so that TX and RX have their own poll functions, which avoids the
need for any redundant checks of the state of the other ring (TX or RX) when
not triggered by an interrupt.

Regards,
Harini