On Wed, Jun 22, 2005 at 07:47:37AM -0400, Jeff Moyer wrote:
> mpm> Hmm. It's conceivable we'll want netdump and kgdb on the same
> mpm> interface so we'll have to address this eventually..
>
> Well, do you want to address it eventually, or now? As I said, it's never
> bitten anyone before.
Later's fine. I just don't want to design it out by accident again.
> >> struct netpoll_info {
> >> spinlock_t poll_lock;
> >> int poll_owner;
> >> int rx_flags;
> >> spinlock_t rx_lock;
> >> struct netpoll *rx_np; /* netpoll that registered an rx_hook */
> >> };
> >>
> >> This is the structure which gets pointed to by the net_device. All of the
> >> flags and locks which are specific to the INTERFACE go here. Any variables
> >> which must be kept per struct netpoll were left in the struct netpoll. So
> >> now, we have a cleaner separation of data and its scope.
> >>
> >> Since we never really supported having more than one struct netpoll
> >> register an rx_hook, I got rid of the rx_list. This is replaced by a
> >> single pointer in the netpoll_info structure (np_rx). We still need to
> >> protect addition or removal of the rx_np pointer, and so keep the lock
> >> (rx_lock). There is one lock per struct net_device, and I am certain that
> >> it will be 0 contention, as rx_np will only be changed during an insmod or
> >> rmmod. If people think this would be a good rcu candidate, let me know and
> >> I'll change it to use that locking scheme.
>
> mpm> It might be simpler to have a single lock here..?
>
> Maybe. You can't really have netpoll code running on multiple cpus at the
> same time, right? This is the rx path, remember, so the other cpu should
> be spinning on the poll_lock.
>
> Keeping separate locks would allow you to unregister a struct netpoll
> associated with another net device without causing lock contention. This
> is a very minor win, obviously.
>
> I still feel like this npinfo struct is the right place for this, though.
> If you're strongly opposed to that, I'll change it.
No, certainly having it in npinfo makes sense. I just was wondering if
we really need two locks in there.
> >> + spin_lock_irqsave(&npinfo->rx_lock, flags);
> >> + if (npinfo->rx_np->dev == skb->dev)
> >> + np = npinfo->rx_np;
> >> + spin_unlock_irqrestore(&npinfo->rx_lock, flags);
>
> mpm> And I think that means we don't need the lock here either.
>
> Sure we do. We need to protect against rmmod's.
How can we have an rmmmod when we're trapped?
> >> static inline int netpoll_rx(struct sk_buff *skb)
> >> {
> >> - return skb->dev->np && skb->dev->np->rx_flags && __netpoll_rx(skb);
> >> + struct netpoll_info *npinfo = skb->dev->npinfo;
> >> + unsigned long flags;
> >> + int ret = 0;
> >> +
> >> + if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
> >> + return 0;
> >> +
> >> + spin_lock_irqsave(&npinfo->rx_lock, flags);
> >> + /* check rx_flags again with the lock held */
> >> + if (npinfo->rx_flags && __netpoll_rx(skb))
> >> + ret = 1;
> >> + spin_unlock_irqrestore(&npinfo->rx_lock, flags);
> >> +
> >> + return ret;
> >> }
>
> mpm> This is perhaps a problem due to cache line bouncing. Perhaps we can
> mpm> use an atomic op and a memory barrier instead?
>
> It really should be a 0 contention lock. Let's not optimize something that
> doesn't need it. If we find that it causes problems, I'll be more than
> happy to fix it.
Ok, fair enough.
--
Mathematics is the supreme nostalgia of our time.
|