Re: [Madwifi-devel] ath_rx_tasklet/ath_rxbuf_init possible potential small bug

[Sam L. <sa...@er...>]

David Beberman wrote:
> I think I just noticed a small bug in the madwifi rx path.
> I grabbed the latest CVS code to make sure it is still the same.
> 
> Here is the situation:
> In ath_rx_tasklet, the main loop attempts to read as many
> completed rx packets as possible.  For each one, it grabs the
> skb from the rx descriptor, decides what to do with it, and then
> puts another skb into the rx descriptor.
> To allocate another skb the ath_rxbuf_init() call is used at the
> bottom of the do/while loop.
> 
> The problem is that if the ath_rxbuf_init() call fails, the
> loop stops, leaving the rx descriptor without an skb to receive
> packets into.  This doesn't cause a crash.  Instead what happens is
> that every time this rx descriptor is checked, if there isn't an skb
> attached, it is skipped by the ath_rx_tasklet.  That can be seen
> in as the if() statement with a continue at the top of the loop.
> I expect  the hardware skips it simply because there isn't any buffer 
> attached to the
> descriptor.  Essentially this becomes a lost buffer descriptor.

ath_rxbuf_init links the replenished rx descriptor to the end of the 
list so if no skbuf can be allocated it simply never gets linked in and 
the hardware never sees it.

> 
> The problem will only show up if the dev_alloc_skb() call, in
> ath_rxbuf_init fails,
> which probably doesn't happen except in extreme low memory
> conditions.

Yes, this is an issue; thank you for pointing it out.

> 
> Another odd behavior here is that whenever dev_alloc_skb fails,
> the ath_rx_tasklet do/while loop will exit even if there are more
> packets to receive.

This is intentional.  Since it wasn't possible to replenish the skbuf it 
seemed like a bad idea to keep going since other skbuf's wouldn't be 
available.  The obvious issue however is that this can leave frames 
waiting for processing.  Since the rx tasklet is only entered when 
there's rx traffic this may not happen immediately.  If we pursue your 
idea however then we could continue processing descriptors.

> 
> If I'm reading the code right, the solution is fairly simple.
> Create a rx descriptor freelist. Whenever a skb alloc fails,
> put the descriptor on the freelist, instead of back on the
> rxbuf list. Attempt to allocate new skb's for the freelist
> at another time.  Perhaps during tx completion handling,
> and at the top of the ath_rx_tasklet.   Would use a
> spinlock to keep things safe.

Dealing with this in the tx tasklet is probably a bad idea 'cuz of the 
locking.  Better to handle it entirely in the rx tasklet or another context.

> 
> If I'm reading this right, the only time this problem would show
> up is if there were packets being received while the system
> was under a heavy enough load that there was no more memory
> available for the skb's.  Since I expect that there would be other
> weird behavior at that point, this probably isn't much of a bug,
> but thought I'd mention it anyways.

There is a counter for when this happens so you can find out.

	Sam