Yes Jan. I believe it should be counted as tx-dropped on the tap interface.

Warm Regards,
Vishal Ajmera

-----Original Message-----
From: Jan Scheurich 
Sent: Monday, January 15, 2018 5:56 PM
To: Vishal Deep Ajmera <[email protected]>; Flavio Leitner 
<[email protected]>; [email protected]
Cc: Rohith Basavaraja <[email protected]>
Subject: RE: [ovs-dev] [PATCH] netdev-linux: do not send packets to down tap 
ifaces.

Hi Vishal,

I assume you want to count these packets as "tx dropped" on the tap interface 
ports in DOWN state, right?

BR, Jan

> -----Original Message-----
> From: [email protected] 
> [mailto:[email protected]] On Behalf Of Vishal Deep 
> Ajmera
> Sent: Monday, 15 January, 2018 11:27
> To: Flavio Leitner <[email protected]>; [email protected]
> Subject: Re: [ovs-dev] [PATCH] netdev-linux: do not send packets to down tap 
> ifaces.
> 
> Hi Flavio,
> 
> Thank you for looking into this issue. It certainly improves the 
> situation for broadcast frames when we have multiple tap ports on the bridge 
> which are in DOWN state.
> 
> However, I see an issue with the patch. It does not handle device 
> statistics. Earlier even when tap-port was DOWN, we would make a write 
> system-call and kernel drops the packets which also accounts it in device 
> statistics (drop counter). But now, since we skip sending packets to kernel 
> then we will have to adjust it in netdev-linux. Can you modify the patch 
> accordingly ?
> 
> Warm Regards,
> Vishal Ajmera
> 
> -----Original Message-----
> From: [email protected] 
> [mailto:[email protected]] On Behalf Of Flavio Leitner
> Sent: Wednesday, January 10, 2018 9:37 PM
> To: [email protected]
> Cc: Flavio Leitner <[email protected]>
> Subject: [ovs-dev] [PATCH] netdev-linux: do not send packets to down tap 
> ifaces.
> 
> Today OVS pushes packets to the TAP interface ignoring its current 
> state. That works because the kernel will return -EIO when it's not UP and 
> OVS will just ignore that as it is not an OVS issue.
> 
> However, it causes a huge impact when broadcasts happen when using userspace 
> datapath accelerated with DPDK (e.g.: action NORMAL).
> This patch improves the situation by checking the TAP's interface state 
> before issueing any syscall.
> 
> However, there might be use-cases moving interfaces to other 
> networking namespaces and in that case, OVS can't retrieve the iface 
> state (sets it to DOWN). That would stop the traffic breaking the use-case. 
> This patch relies on netlink notifications to find out if the device is local 
> or not. When it's local, the device state is checked otherwise it will behave 
> as before.
> 
> Signed-off-by: Flavio Leitner <[email protected]>
> ---
>  lib/netdev-linux.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 
> ccb6def6c..53b08bebd 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -502,6 +502,7 @@ struct netdev_linux {
> 
>      /* For devices of class netdev_tap_class only. */
>      int tap_fd;
> +    bool present;            /* If the device is present in the namespace */
>  };
> 
>  struct netdev_rxq_linux {
> @@ -750,8 +751,10 @@ netdev_linux_update(struct netdev_linux *dev,
>              dev->ifindex = change->if_index;
>              dev->cache_valid |= VALID_IFINDEX;
>              dev->get_ifindex_error = 0;
> +            dev->present = true;
>          } else {
>              netdev_linux_changed(dev, change->ifi_flags, 0);
> +            dev->present = false;
>          }
>      } else if (rtnetlink_type_is_rtnlgrp_addr(change->nlmsg_type)) {
>          /* Invalidates in4, in6. */
> @@ -1234,6 +1237,16 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,  {
>      struct netdev_linux *netdev = netdev_linux_cast(netdev_);
>      struct dp_packet *packet;
> +
> +    /* The Linux tap driver returns EIO if the device is not up,
> +     * so if the device is not up, don't waste time sending it.
> +     * However, if the device is in another network namespace
> +     * then OVS can't retrieve the state. In that case, send the
> +     * packets anyway. */
> +    if (netdev->present && !(netdev->ifi_flags & IFF_UP)) {
> +        return 0;
> +    }
> +
>      DP_PACKET_BATCH_FOR_EACH (packet, batch) {
>          size_t size = dp_packet_size(packet);
>          ssize_t retval;
> --
> 2.14.3
> 
> 
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to