Message ID | 1329998028.15610.14.camel@edumazet-laptop |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, 2012-02-23 at 12:53 +0100, Eric Dumazet wrote: > Hmm, I am not sure we still need local_bh_disable()/local_bh_enable() in > kmap_skb_frag()/ kunmap_skb_frag() anymore after commit 3e4d3af501ccc > (mm: stack based kmap_atomic() ) The only thing to consider is keeping the total stack size under control, this is somewhat non-trivial since its non-obvious what all nests. That said, you're probably right, and we do have a WARN in there (dependent on CONFIG_DEBUG_HIGHMEM) that yells if we exceed the available stack size. The more 'interesting' exercise is determining a better upper bound on the stack size and updating kmap_types.h accordingly. > diff --git a/net/core/kmap_skb.h b/net/core/kmap_skb.h > index 81e1ed7..06be5ee 100644 > --- a/net/core/kmap_skb.h > +++ b/net/core/kmap_skb.h > @@ -2,18 +2,10 @@ > > static inline void *kmap_skb_frag(const skb_frag_t *frag) > { > -#ifdef CONFIG_HIGHMEM > - BUG_ON(in_irq()); > - > - local_bh_disable(); > -#endif > return kmap_atomic(skb_frag_page(frag), KM_SKB_DATA_SOFTIRQ); > } > > static inline void kunmap_skb_frag(void *vaddr) > { > kunmap_atomic(vaddr, KM_SKB_DATA_SOFTIRQ); > -#ifdef CONFIG_HIGHMEM > - local_bh_enable(); > -#endif > } The nicer patch would of course be a patch that does: s/kunmap_skb_frag/kunmap_atomic/ s/kmap_skb_frag(\([^)]*\))/kmap_atomic(skb_frag_page(\1))/ There is no need to retain the KM_* argument and the 'helper' functions are quite pointless at that point. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le jeudi 23 février 2012 à 14:22 +0100, Peter Zijlstra a écrit : > On Thu, 2012-02-23 at 12:53 +0100, Eric Dumazet wrote: > > Hmm, I am not sure we still need local_bh_disable()/local_bh_enable() in > > kmap_skb_frag()/ kunmap_skb_frag() anymore after commit 3e4d3af501ccc > > (mm: stack based kmap_atomic() ) > > The only thing to consider is keeping the total stack size under > control, this is somewhat non-trivial since its non-obvious what all > nests. > > That said, you're probably right, and we do have a WARN in there > (dependent on CONFIG_DEBUG_HIGHMEM) that yells if we exceed the > available stack size. > > The more 'interesting' exercise is determining a better upper bound on > the stack size and updating kmap_types.h accordingly. > I would say its the same logic than crypto : We might use at most two contexts for SKB frags : USER or SOFTIRQ We probably can remove KM_SKB_DATA_SOFTIRQ slot and use fact that this kmap user can reuse existing USER/SOFTIRQ slots > > diff --git a/net/core/kmap_skb.h b/net/core/kmap_skb.h > > index 81e1ed7..06be5ee 100644 > > --- a/net/core/kmap_skb.h > > +++ b/net/core/kmap_skb.h > > @@ -2,18 +2,10 @@ > > > > static inline void *kmap_skb_frag(const skb_frag_t *frag) > > { > > -#ifdef CONFIG_HIGHMEM > > - BUG_ON(in_irq()); > > - > > - local_bh_disable(); > > -#endif > > return kmap_atomic(skb_frag_page(frag), KM_SKB_DATA_SOFTIRQ); > > } > > > > static inline void kunmap_skb_frag(void *vaddr) > > { > > kunmap_atomic(vaddr, KM_SKB_DATA_SOFTIRQ); > > -#ifdef CONFIG_HIGHMEM > > - local_bh_enable(); > > -#endif > > } > > The nicer patch would of course be a patch that does: > > s/kunmap_skb_frag/kunmap_atomic/ > s/kmap_skb_frag(\([^)]*\))/kmap_atomic(skb_frag_page(\1))/ > > There is no need to retain the KM_* argument and the 'helper' functions > are quite pointless at that point. > > Well, definitely a cleanup is possible, but probably not suitable for 3.3 and stable kernels ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-02-23 at 15:21 +0100, Eric Dumazet wrote: > Well, definitely a cleanup is possible, but probably not suitable for > 3.3 and stable kernels ? Nah, that'd be better suited for a development branch like :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/kmap_skb.h b/net/core/kmap_skb.h index 81e1ed7..06be5ee 100644 --- a/net/core/kmap_skb.h +++ b/net/core/kmap_skb.h @@ -2,18 +2,10 @@ static inline void *kmap_skb_frag(const skb_frag_t *frag) { -#ifdef CONFIG_HIGHMEM - BUG_ON(in_irq()); - - local_bh_disable(); -#endif return kmap_atomic(skb_frag_page(frag), KM_SKB_DATA_SOFTIRQ); } static inline void kunmap_skb_frag(void *vaddr) { kunmap_atomic(vaddr, KM_SKB_DATA_SOFTIRQ); -#ifdef CONFIG_HIGHMEM - local_bh_enable(); -#endif }