Re: Followup to netpoll issues

Mark Broadbent [off-list ref] :
[...]

No need, if netpoll_cleanup is called whilst a retry is pending the tx
list lock is taken.  All the references to the netpoll pointer are
deleted from the pending list before the lock is released.

I may be a bit dense but:

t0             : tx_retry_wq().queue_delayed_work(tx_wq, &tx_wq_obj, HZ/100);

t0 + 10*HZ/1000: tx_retry_wq() is done (it was not fast for sure)

t0 + 20*HZ/1000: netpoll_cleanup() + module removal

t0 + HZ/100    : tx_retry_wq() <- Where are its code and data ?

[...]

quoted

However netpoll_send_skb can be called through:
netpoll_rx -> arp_reply -> netpoll_send_skb
Here I fail to see why netpoll_cleanup could not happen at the same time
and issue np->dev = NULL;

Could this solved by having a per netpoll structure rw_lock to protect
against changing elements in the structure?

e.g. 
struct netpoll {
	...
	rwlock_t netpoll_lock;
};

write_lock_irqsave(&np->netpoll_lock, flags);
np->dev = NULL;
write_unlock_irqsave(&np->netpoll_lock, flags);

I am not sure that a fine coarsed lock is necessary since there
will mostly be readers and netpoll_cleanup should be rare.

[...]

Is it safe to issue spin_lock_irqsave/spin_unlock_restore in this
context?  Reason: What if it wasn't netconsole calling netpoll_send_udp
and not with local interrupts disabled?

The irq state is saved and restored.

Did I get the question right ?

[...]

I've attached a revised version.

Thanks for you comments (and Matts)

The malloc() in netpoll_send_skb() is heavy-weight and it is not that
uncommon to fail allocation in network context: preallocate when the
np is first set up. It will make netpoll_send_skb() lighter.

Issue schedule_delayed_work() in netpoll_send_skb() only if when the
task is not already scheduled ? It is an information you can get/set
while tx_list_lock is taken.

I'll read the patch (or any updated version) more closely once I have
recovered my sleep quota. It should not be too hard to isolate the
queueing specific stuff from the real fixes.

--
Ueimor