On Tue, Sep 16, 2014 at 11:04:13AM +0100, Martin Townsend wrote:
> Hi Alex,
>
> On the lowpan_give_skb_to_devices change.
>
> As we are iterating over a list of lowpan_devices and could potentially copy the skb more than once, what happens if the first device returns NET_RX_DROP and then the second time it return NET_RX_SUCCESS? The stat variable is overwritten so stat only ever reflects the return value of netif_rx for the last device?
>
> Maybe it's better to completely remove the if else at the end and always consume the skb? For the case whereskb_copy fails then we should kfree_skb,
> e.g.
>
> static int lowpan_give_skb_to_devices(struct sk_buff *skb,
> struct net_device *dev)
> {
> struct lowpan_dev_record *entry;
> struct sk_buff *skb_cp;
> int stat = NET_RX_SUCCESS;
>
> rcu_read_lock();
> list_for_each_entry_rcu(entry, &lowpan_devices, list)
> if (lowpan_dev_info(entry->ldev)->real_dev == skb->dev) {
> skb_cp = skb_copy(skb, GFP_ATOMIC);
> if (!skb_cp) {
> kfree_skb(skb);
> rcu_read_unlock();
> return NET_RX_DROP;
> }
>
> skb_cp->dev = entry->ldev;
> stat = netif_rx(skb_cp);
here we should do a:
if (stat == NET_RX_DROP)
kfree_skb(skb_cp);
or? It doesn't deliver and then we "could" lost the pointer.
> }
> rcu_read_unlock();
>
> consume_skb(skb);
>
> return stat;
> }
>
>
> what are your thoughts?
>
for consume_skb:
for me it's ok to make this behaviour. We never deliver the skb, always
skb_cp. So if we are before the deliver call (netif_rx) this should
never failed and we should consume the skb from which we did some copies.
btw.
I see now that's skb_copy... mhhh. But this another issue. There exist
skb_clone and skb_copy. skb_clone make a copy of struct sk_buff and data
buffer is shared. I am currently not sure if we also can use a skb_clone
here instead skb_copy, because the IPv6 doesn't manipulate the data buffer
(I think it doesn't change the data buffer -> only parse) I need to think
more about this, just a performance hint. But I really also doesn't know
what sense makes multiple lowpan devices for one wpan interface. :-)
- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
