On 15-09-15 09:11 PM, Alexei Starovoitov wrote:
> On 9/15/15 8:10 PM, John Fastabend wrote:
>> Nice, I like this. But just to be sure I read this correctly this will
>> only work on the ingress qdisc for now right? To get the tx side working
>> will require a bit more care.
>
> correct.
> For egress I'm waiting for Daniel to resubmit his preclassifier patch
> and I'll hook this skb_do_redirect() there as well.
> Other options are also possible, but preclassifier looks the best for
> this purpose, since it's lockless.
>
Great, works for me. One other question/observation,
+int skb_do_redirect(struct sk_buff *skb)
+{
[...]
+
+ if (unlikely(!(dev->flags & IFF_UP))) {
+ kfree_skb(skb);
+ return -EINVAL;
+ }
The IFF_UP check is not needed as best I can tell, the dev_queue_xmit()
will check if the qdisc is active and the dev_forward_skb() path will
do a !netif_running check in enqueue_to_backlog() call.
Looks like you can remove the check. I would prefer to let the stack
handle this case using normal mechanisms.
I had to do a bit of tracking but netif_running check equates roughly
to your IFF_UP case via,
> __dev_change_flags()
> [...]
> if ((old_flags ^ flags) & IFF_UP)
> ret = ((old_flags & IFF_UP) ? __dev_close :
> __dev_open)(dev);
>
>
> __dev_close()
> [...]
> __dev_close_many()
>
> __dev_close_many()
> [...]
> clear_bit(__LINK_STATE_START, &dev->state);
Seem reasonable? Or did you put it there to work around some specific
case I'm missing?
.John
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
