[Cbe-oss-dev] [patch 15/16] powerpc and x86_64: Initial port of PCIe endpoint device driver for Axon

[Arnd Bergmann]

On Tuesday 25 March 2008, Murali Iyer wrote:
> On Mon, 2008-03-17 at 05:45 +0100, Arnd Bergmann wrote:
> 
> > I think it would be good to also post this driver to the 
> > netdev at vger.kernel.org mailing list next time, because we don't
> > have many network experts around here.
> OK. We do have experts like Ben H, isn't it?

Right, but network drivers need to pass through a review on
netdev to get accepted. It's always good to have many people
looking over stuff instead of sending everything to the same
few developers.
> 
> > Do I see this right that you call axon_enable_notify_callback() to switch
> > on/off the local delivery of callback interrupts, but not the actual calling
> > of the interrupt handler?
> Correct. 

Ok, that doesn't help much then. You should really try to get the
originator of the interrupts to stop sending them instead. The most
significant problem with interrupts is all the time required to
get the interrupt handler to start and then get back to the code
that the machine was executing before.

> > 
> > > +static void apnet_tx_timeout(struct net_device *ndev)
> > > +{
> > > +       struct apnet *adev = netdev_priv(ndev);
> > > +
> > > +       if (apnet_tx_ring_status(adev))
> > > +               return;
> > > +
> > > +       if (adev->astats.tx_dma_err)    /* return if we got DMA error */
> > > +               return;
> > > +
> > > +       if (apnet_get_tx_bd(adev))
> > > +               netif_wake_queue(ndev); /* found free slot to xmit */
> > > +       else
> > > +               axon_send_remote_notification(APNET_MSG_TYPE,
> > > adev->if_num);
> > > +       return; 
> > 
> > If the DMA failed, do you really want to send a remote notification?
> > Doesn't that cause the broken packet to get received by the remote
> > side?
> This is not for DMA failure alone.. if remote side is descriptors are
> full for various reasons like slow cpu speed, busy etc. this will kick
> start the rx. not come across the situation but as a insurance won't
> hurt.

I disagree, code that is only meant as an insurance if something goes
wrong in other code is harmful, because it will hide bugs. At least
have a warning message here if you detect a condition that should not
be possible.

> > If I read this correctly, you generate a local interrupt for every frame
> > you transmit, and from this handler, generate another remote interrupt
> > to notify of the packet being transmitted if that is enabled?
> > 
> > IIRC, you get a lot of tx interrupts this way, and it would be good
> > to reduce this number. Can you send the dma-complete event to the
> > remote end instead? That would first of all reduced the latency
> > of the initial packet rx, and also mean that you can consolidate
> > multiple tx-complete events in a single interrupt sent back from
> > the receiving end.
> This driver uses DMA driver to xmit data. DMA driver could be explored
> optimized to reduce the DMA complete interrupts. from apnet perspective
> it doesn't generate too many interrupts because of NAPI for RX side.
> Even though it appears apnet generates every interrupt for remote,
> axon_send_remote_notification() is checked by the base driver and sends
> the remote notification interrupt only if
> axon_enable_remote_notification() was called otherwise just returns.
> This is done because interrupt to be shared between multiple clients.

Ok, so you can actually avoid most of the receive interrupts, which
is good, but as long as you still need to get an interrupt for every
packet you transmit, you can never be better than 50% of the worst case
number of interrupts (one on each side, per packet).

Ideally, a constant stream of data from one side to the other should
generate almost no interrupts at all, as there is always more work
to do by the network softirq.

Many drivers trigger netif_rx_schedule() (right: rx, not tx) from the
transmit interrupt, and do all the work in the NAPI poll() function
for both rx and tx.

> > remember to make these messages pr_debug at some point. When debugging,
> > it shouldn't make a difference, and later they get annoying.
> This will be logged only when user changes mac address using something
> like "ifconfig apnet? hw ether <MAC-ADDRESS> and expect often.
> My intention is to keep it as pr_info.

Please don't log user-triggered events, the user already knows about
them. Instead, you should only log asynchronous events that come from
the hardware, e.g. unexpected behaviour of the remote side.

> > 
> > > +#define pr_err(fmt, arg...) \
> > > +	printk(KERN_ERR fmt, ##arg)
> > 
> > This is already defined in kernel.h
> Thanks for later kernel.h, it isn't in 2.6.23.x

The best way to deal with issues like this is to have a small compat.h file
in your driver directory that provides all the new interfaces when building
on an old kernel. You can then add

CFLAGS += -include compat.h

in your Makefile when building on the old kernel, and leave that out on
the newer ones.

	Arnd <><