From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 17572A0C53; Fri, 3 Sep 2021 11:46:10 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8D5F740DF7; Fri, 3 Sep 2021 11:46:09 +0200 (CEST) Received: from mail-ed1-f43.google.com (mail-ed1-f43.google.com [209.85.208.43]) by mails.dpdk.org (Postfix) with ESMTP id 869DE40DF4 for ; Fri, 3 Sep 2021 11:46:08 +0200 (CEST) Received: by mail-ed1-f43.google.com with SMTP id s25so7234483edw.0 for ; Fri, 03 Sep 2021 02:46:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=iT6ufiyS4zqXeZLp6E4n5czdaA4u94R+jGsb9OW2v+w=; b=Lc+ld4v92DxxFLYl3wa8NmwrItyoerdFC81ybLvDI4Kc0gY+1RfncCDvU9XAqsRGCT EFHJRTwgcOxP/jSu2GELjDsGG13gLJLJgQ29kc6Yq4Ucy3OSNMQaEH4fsf2FhpgEJX9M PbK5lauqBm6J60GrYF4+2U7veDbfazgta5W9IofU0u5TAd91lz7LME2jIIE7Pkv4nCdk 6rkcXCEpUUNftMsY+ng8K8O+T1JzwFmanVduKyYpUoM7d5G7NLhACllF6snq6NiUvoB3 N6J9/qIY1ooccbcuZaC4aKEXVkFf6r/zFmzyqCPHz814YvaCUeUoLAxtwGxbX/HwXriu z1jA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=iT6ufiyS4zqXeZLp6E4n5czdaA4u94R+jGsb9OW2v+w=; b=lQQu7jEsqPlskLEkMZV+oE6vy5IXAeZEW2ASvp1FSLCIx0GaI0qep/ybdpj+uLTHnJ a39XlIZ69Fx8E8WQXtNqnl2zgbg9eDt1aua9Nwe+0WMAtRB55zGTJ4pnxGLmPZtwzdAI cZzDE+ZrUF4H4xwCPqu+afqtptIn20sW0E1ih+AuHFF/PvmPttGF8LZApmPqFYQdROEz 7xMXMqjWBCp6bGkFFJq4THpD58hYuBFcYWSiQ3U1gJKOdGOd1qlm0+MfZkwuq4Xxmus0 WaK33gZ3BAQ7S9TfLu25iIxMWkp12BWe6tpNE1dZ6nxoH9NjKYATrVW7NlouOHl/keFQ J0/Q== X-Gm-Message-State: AOAM532n9YnP24uEDGjBov0KeP5aYELlmD1J2btfFo1/ZJdse32Svtol iR+yzMM1n8ngKAJSDr9y2MZvW9hlAqucTWnpTyQ= X-Google-Smtp-Source: ABdhPJzfYiW6YD+3kcPbs526pirb9vsnaclQooOTE55IxlgNQRV0acjZcbdKn2bumeT+MxqGwIElslUn1WxdkTgl+sI= X-Received: by 2002:a50:ec97:: with SMTP id e23mr3159583edr.25.1630662368116; Fri, 03 Sep 2021 02:46:08 -0700 (PDT) MIME-Version: 1.0 References: <1629463607-76292-1-git-send-email-tudor.cornea@gmail.com> <1483bf7f-09ec-edd5-1fc1-4b4c81206ae2@intel.com> <20210901143420.5977fd9d@hermes.local> <182a292a-c004-d49b-fc3d-c48dba79206f@intel.com> In-Reply-To: <182a292a-c004-d49b-fc3d-c48dba79206f@intel.com> From: Tudor Cornea Date: Fri, 3 Sep 2021 12:45:57 +0300 Message-ID: To: Ferruh Yigit Cc: linville@tuxdriver.com, dev@dpdk.org, Stephen Hemminger , Andrew Rybchenko , Thomas Monjalon , "jerinj@marvell.com" Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [dpdk-dev] [PATCH] net/af_packet: try to reinsert the stripped vlan tag X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Thanks, I hope I understood correctly the above comments. I'm thinking of adding DEV_RX_OFFLOAD_VLAN_STRIP to dev_info->rx_offload_capa eth_dev_info() +dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP; then populating pmd_internals->vlan_strip with the vlan stripping option that the application requests eth_dev_configure() +internals->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP); >From the internals structure, we could populate a newly-added field in the pkt_rx_queue structure 'vlan_strip' eth_rx_queue_setup() +pkt_q->vlan_strip = internals->vlan_strip; And attempt to re-insert the vlan only if required in eth_af_packet_rx eth_af_packet_rx() +if (!pkt_q->vlan_strip && rte_vlan_insert(&mbuf)) I've attempted a simple benchmark to understand if the change could cause a sizable performance hit. Setup: Tx: vmxnet3 PMD Rx: af_packet (running on top of a vmxnet3 interface) Packet size :68 (packet contains a vlan tag) Rates: Tx - 1.419 Mpps Rx (without vlan insertion) - 1227636 pps Rx (with vlan insertion) - 1220081 pps I don't seem to have a large degradation in terms of packet rate at first glance, but maybe the experiment could be repeated on different setups as I'm using a virtual environment. Would it be reasonable if I send v2 of the patch for review, with the above changes ? On Thu, 2 Sept 2021 at 13:49, Ferruh Yigit wrote: > On 9/1/2021 10:34 PM, Stephen Hemminger wrote: > > On Wed, 1 Sep 2021 22:07:22 +0300 > > Tudor Cornea wrote: > > > >> Indeed, the vlan insertion could be a costly operation. We should > probably > >> do it only if the user specifically asks to have the vlan tag in the > packet. > >> Otherwise, af_packet PMD users might pay a price in terms of performance > >> for something they didn't ask for. > >> > >> I was thinking of avoiding having to change the application in order to > >> re-insert the vlan tag. > >> Doing this operation inside the PMD driver seemed like a good fit. > >> > >> Looking at the netvsc driver (drivers/net/netvsc), the vlan insertion is > >> guarded by a check to hv->vlan_strip > >> > >> if (!hv->vlan_strip && rte_vlan_insert(&m)) { > >> > >> hv->vlan_strip seems to be initialized in hn_dev_configure() in the > >> following way > >> > >> hv->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP); > >> > >> while 'hv' seems to be stored in rte_eth_dev->data->dev_private > >> > >> I am thinking of doing something similar for the af_packet PMD. > >> The 'pmd_internals' structure could potentially hold a field, say > >> vlan_strip', which could be initialized if the application enables the > >> DEV_RX_OFFLOAD_VLAN_STRIP in rxmode->offloads > >> > >> This way, I'm thinking that the application could potentially control > the > >> effect of vlan stripping for the af_packet PMD, in an uniform way, > similar > >> to other PMDs. > >> Would this be considered an acceptable solution ? > >> > >> > >> > >> > >> On Tue, 31 Aug 2021 at 18:31, Ferruh Yigit > wrote: > >> > >>> On 8/20/2021 1:46 PM, Tudor Cornea wrote: > >>>> The af_packet pmd driver binds to a raw socket and allows > >>>> sending and receiving of packets through the kernel. > >>>> > >>>> Since commit bcc6d47903 [1], the kernel strips the vlan tags early in > >>>> __netif_receive_skb_core(), so we receive untagged packets while > >>>> running with the af_packet pmd. > >>>> > >>>> Luckily for us, the skb vlan-related fields are still populated from > the > >>>> stripped vlan tags, so we end up having all the information > >>>> that we need in the mbuf. > >>>> > >>>> We would like to have the the vlan tag inside the mbuf. > >>>> Let's take a shot at it by trying to reinsert the stripped vlan tag. > >>>> > >>> > >>> PMD already sets 'mbuf->vlan_tci' and 'PKT_RX_VLAN | > PKT_RX_VLAN_STRIPPED' > >>> flags, so application can be aware of the vlan tag and can consume it. > >>> > >>> Inserting the vlan tag back to packet is costly, what is the > motivation to > >>> do so? > >>> > >>>> As a side note, something similar was done for the netvsc pmd. > >>>> > >>>> [1] > >>> > https://github.com/torvalds/linux/commit/bcc6d47903612c3861201cc3a866fb604f26b8b2 > > >>>> > >>>> Signed-off-by: Tudor Cornea > > > > The netvsc PMD has to handle some subtle cases where VLAN stripping > > is done by the VF but the slow path over vmbus does not. > > Since most traffic goes over the VF path, it makes sense for the > > netvsc PMD to advertise and handle VLAN stripping even if it has > > to do it in software. > > > > Ferruh is right the mbuf generated by current AF_PACKET PMD is > > valid with VLAN stripped correctly. I think you are also correct > > that the stripping needs to be controllable by the application. > > And yes the kernel always strips the VLAN; there is no option > > to tell socket(AF_PACKET) not to do that. > > > > When application doesn't set VLAN_STRIP offload, expectation is VLAN tag > to be > in the packet and no additional work is done. > > But that is not the case for af_packet. > If your change is applied, not requesting any offload, default confing, may > cause unintended work for af_packet, since it will insert the already > stripped > vlan tag back. > > And we don't have a way to say any specific offload can't be disabled by > the > PMD/device, although we hit this case a few times previously. > Proper fix can be adding this support to offloads, but it is more invasive > change. + Andrew, Thomas, Jerin for this discussion. > > > For short term at least 'DEV_RX_OFFLOAD_VLAN_STRIP' offload should be > added to > the af_packet capability. > It is also possible to set this offload in the config by PMD itself even > application doesn't request it, to be correct in the config. Not sure how > much > it helps to applications (there is a new API proposed this release to get > config > to application, perhaps after configuration step app can request the > config and > recognize that VLAN_STRIP offload is set by PMD, but this is some > overhead). >