[dpdk-dev] librte_pmd_af_packet: add compile-time checks for kernel-specific options

Message ID 1417729325-9410-1-git-send-email-linville@tuxdriver.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

John W. Linville Dec. 4, 2014, 9:42 p.m. UTC
  This allows the PMD to compile with kernels that don't support the
options in question.  The "#if defined(...)" lines are a bit ugly,
but I don't know of any better way to accomplish the task.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 lib/librte_pmd_af_packet/rte_eth_af_packet.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
  

Comments

Neil Horman Dec. 5, 2014, 3:46 a.m. UTC | #1
On Thu, Dec 04, 2014 at 04:42:05PM -0500, John W. Linville wrote:
> This allows the PMD to compile with kernels that don't support the
> options in question.  The "#if defined(...)" lines are a bit ugly,
> but I don't know of any better way to accomplish the task.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> index 3b5bd5840f51..d0fb3eb32e44 100644
> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> @@ -444,9 +444,9 @@ rte_pmd_init_internals(const char *name,
>  	struct tpacket_req *req;
>  	struct pkt_rx_queue *rx_queue;
>  	struct pkt_tx_queue *tx_queue;
> -	int rc, tpver, discard, bypass;
> +	int rc, qsockfd, tpver, discard;
>  	unsigned int i, q, rdsize;
> -	int qsockfd, fanout_arg;
> +	int fanout_arg __rte_unused, bypass __rte_unused;
>  
>  	for (k_idx = 0; k_idx < kvlist->count; k_idx++) {
>  		pair = &kvlist->pairs[k_idx];
> @@ -519,9 +519,13 @@ rte_pmd_init_internals(const char *name,
>  	sockaddr.sll_protocol = htons(ETH_P_ALL);
>  	sockaddr.sll_ifindex = (*internals)->if_index;
>  
> +#if defined(PACKET_FANOUT)
>  	fanout_arg = (getpid() ^ (*internals)->if_index) & 0xffff;
> -	fanout_arg |= (PACKET_FANOUT_HASH | PACKET_FANOUT_FLAG_DEFRAG |
> -	               PACKET_FANOUT_FLAG_ROLLOVER) << 16;
> +	fanout_arg |= (PACKET_FANOUT_HASH | PACKET_FANOUT_FLAG_DEFRAG) << 16;
> +#if defined(PACKET_FANOUT_FLAG_ROLLOVER)
> +	fanout_arg |= PACKET_FANOUT_FLAG_ROLLOVER << 16;
> +#endif
> +#endif
>  
>  	for (q = 0; q < nb_queues; q++) {
>  		/* Open an AF_PACKET socket for this queue... */
> @@ -553,6 +557,7 @@ rte_pmd_init_internals(const char *name,
>  			goto error;
>  		}
>  
> +#if defined(PACKET_QDISC_BYPASS)
>  		bypass = 1;
>  		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_QDISC_BYPASS,
>  				&bypass, sizeof(bypass));
> @@ -563,6 +568,7 @@ rte_pmd_init_internals(const char *name,
>  			        pair->value);
>  			goto error;
>  		}
> +#endif
>  
>  		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_RX_RING, req, sizeof(*req));
>  		if (rc == -1) {
> @@ -623,6 +629,7 @@ rte_pmd_init_internals(const char *name,
>  			goto error;
>  		}
>  
> +#if defined(PACKET_FANOUT)
>  		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_FANOUT,
>  				&fanout_arg, sizeof(fanout_arg));
>  		if (rc == -1) {
> @@ -631,6 +638,7 @@ rte_pmd_init_internals(const char *name,
>  				"for %s\n", name, pair->value);
>  			goto error;
>  		}
> +#endif
>  	}
>  
>  	/* reserve an ethdev entry */
> -- 
> 1.9.3
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>
  
Thomas Monjalon Dec. 5, 2014, 9:30 a.m. UTC | #2
Hi John,

2014-12-04 16:42, John W. Linville:
> This allows the PMD to compile with kernels that don't support the
> options in question.  The "#if defined(...)" lines are a bit ugly,
> but I don't know of any better way to accomplish the task.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

Is it possible to enable compilation of this PMD in default configuration?
I guess it shouldn't fail anymore with this patch?
  
John W. Linville Dec. 5, 2014, 4:24 p.m. UTC | #3
On Fri, Dec 05, 2014 at 10:30:53AM +0100, Thomas Monjalon wrote:
> Hi John,
> 
> 2014-12-04 16:42, John W. Linville:
> > This allows the PMD to compile with kernels that don't support the
> > options in question.  The "#if defined(...)" lines are a bit ugly,
> > but I don't know of any better way to accomplish the task.
> > 
> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> > ---
> >  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> Is it possible to enable compilation of this PMD in default configuration?
> I guess it shouldn't fail anymore with this patch?

Yes, I believe so.  I checked the kernel git history for when each
definition was included.  Everything else was older than the 2.6.32
kernel.

I think you can reenable this PMD in the common_linuxapp config file
if you like.  I'll send a patch if you prefer for me to do so.

John
  
Thomas Monjalon Dec. 5, 2014, 4:37 p.m. UTC | #4
2014-12-05 11:24, John W. Linville:
> On Fri, Dec 05, 2014 at 10:30:53AM +0100, Thomas Monjalon wrote:
> > Hi John,
> > 
> > 2014-12-04 16:42, John W. Linville:
> > > This allows the PMD to compile with kernels that don't support the
> > > options in question.  The "#if defined(...)" lines are a bit ugly,
> > > but I don't know of any better way to accomplish the task.
> > > 
> > > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> > > ---
> > >  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 16 ++++++++++++----
> > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > Is it possible to enable compilation of this PMD in default configuration?
> > I guess it shouldn't fail anymore with this patch?
> 
> Yes, I believe so.  I checked the kernel git history for when each
> definition was included.  Everything else was older than the 2.6.32
> kernel.
> 
> I think you can reenable this PMD in the common_linuxapp config file
> if you like.  I'll send a patch if you prefer for me to do so.

OK, I'm going do it.

Thanks
  
Aashima Arora Dec. 5, 2014, 5:29 p.m. UTC | #5
Hi,
I was trying to access the pci config space(BAR) of the virtual function device visible in the virtual machine, similar to what DPDK does on host via both UIO and VFIO. Did the following steps.

1. Bound PF Drivers to ixgbe and spawned virtual function drivers , bound them to vfio-pci and set their mac addresses via ip link.  Ran Qemu and assigned the VF Device using vfio-pci device assignment and initialized the virtual machine.
insmod igb max_vfs=2

./x86_64-softmmu/qemu-system-x86_64 -cpu host -boot c -hda /home/vm-images/vm2.imgsnapshot -m 2048M -smp 2 --enable-kvm -name 'vm2' -vnc :2 -pidfile /tmp/vm2.pid -driile=fat:rw:/tmp/share,snapshot=off -device vfio-pci,host=01:10.1,id=net1


2.  The VF Device was visible with another pci address.

00:04.0 Ethernet controller: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network
Connection (rev 01)

Further ran DPDK testpmd on top of VM but bound the virtual function driver to igb_uio instead of vfio-pci. It ran successfully.  The ixgbevf pmd driver is able to access the BAR registers in pci_uio_map_resource via mmaping somewhere close to hugepages. I was not able to bind the virtual function driver in VM to vfio-pci and hence DPDK would not be able to run with VFIO enabled as it complains of no IOMMU support. I also believe that there is little logic in binding the vf device to vfio-pci again since qemu has already taken care of it and hardware support is involved.

So my questions are
a. vfio is meant to be  a replacement for both uio and device assignment for qemu. This doesnt seem simultaneous. Comment?
b. Is there any way to access VF device  using VFIO in guest userspace? Have you run DPDK in guest machine with VFIO support enabled and all dependent modules inserted and did it work?
c. Is igb_uio or uio_pci_generic in future the only way  to access the device in guest userspace?




"DISCLAIMER: This message is proprietary to Aricent and is intended solely for the use of the individual to whom it is addressed. It may contain privileged or confidential information and should not be circulated or used for any purpose other than for what it is intended. If you have received this message in error, please notify the originator immediately. If you are not the intended recipient, you are notified that you are strictly prohibited from using, copying, altering, or disclosing the contents of this message. Aricent accepts no responsibility for loss or damage arising from the use of the information transmitted by this email including damage from virus."
  
Thomas Monjalon Dec. 5, 2014, 6:08 p.m. UTC | #6
Hi,

Please don't reply to a previous email to start a different topic.
You can check how it looks here:
	http://thread.gmane.org/gmane.comp.networking.dpdk.devel

> "DISCLAIMER: This message is proprietary to Aricent and is intended solely for the use of the individual to whom it is addressed. It may contain privileged or confidential information and should not be circulated or used for any purpose other than for what it is intended. If you have received this message in error, please notify the originator immediately. If you are not the intended recipient, you are notified that you are strictly prohibited from using, copying, altering, or disclosing the contents of this message. Aricent accepts no responsibility for loss or damage arising from the use of the information transmitted by this email including damage from virus."

Please remove this disclaimer.
It's not specifically against you but it's at least the tenth time I say this
to an Aricent guy. This disclaimer should force us to remove your message
(even in the archives). So we should maybe block all the emails from aricent.com.
  
Neil Horman Dec. 5, 2014, 6:42 p.m. UTC | #7
On Fri, Dec 05, 2014 at 07:08:55PM +0100, Thomas Monjalon wrote:
> Hi,
> 
> Please don't reply to a previous email to start a different topic.
> You can check how it looks here:
> 	http://thread.gmane.org/gmane.comp.networking.dpdk.devel
> 
> > "DISCLAIMER: This message is proprietary to Aricent and is intended solely for the use of the individual to whom it is addressed. It may contain privileged or confidential information and should not be circulated or used for any purpose other than for what it is intended. If you have received this message in error, please notify the originator immediately. If you are not the intended recipient, you are notified that you are strictly prohibited from using, copying, altering, or disclosing the contents of this message. Aricent accepts no responsibility for loss or damage arising from the use of the information transmitted by this email including damage from virus."
> 
> Please remove this disclaimer.
> It's not specifically against you but it's at least the tenth time I say this
> to an Aricent guy. This disclaimer should force us to remove your message
> (even in the archives). So we should maybe block all the emails from aricent.com.
> 
+1.  These disclaimers are not only most likely toothless (though IANAL), but it
does place the community in the position of either needing to take the effort to
scrub your emails from the archives to be in compliance with the disclamer, or
to take the legal risk that such a disclaimer actually has merit.  If your
company can't abide by you removing such a notice, than that company can't
participate in open source.

Neil

> -- 
> Thomas
>
  
Thomas Monjalon Dec. 5, 2014, 9:14 p.m. UTC | #8
2014-12-05 17:37, Thomas Monjalon:
> 2014-12-05 11:24, John W. Linville:
> > On Fri, Dec 05, 2014 at 10:30:53AM +0100, Thomas Monjalon wrote:
> > > Hi John,
> > > 
> > > 2014-12-04 16:42, John W. Linville:
> > > > This allows the PMD to compile with kernels that don't support the
> > > > options in question.  The "#if defined(...)" lines are a bit ugly,
> > > > but I don't know of any better way to accomplish the task.
> > > > 
> > > > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> > > > ---
> > > >  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 16 ++++++++++++----
> > > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > Is it possible to enable compilation of this PMD in default configuration?
> > > I guess it shouldn't fail anymore with this patch?
> > 
> > Yes, I believe so.  I checked the kernel git history for when each
> > definition was included.  Everything else was older than the 2.6.32
> > kernel.
> > 
> > I think you can reenable this PMD in the common_linuxapp config file
> > if you like.  I'll send a patch if you prefer for me to do so.
> 
> OK, I'm going do it.

Applied with default config change to enable the PMD.

Thanks
  

Patch

diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
index 3b5bd5840f51..d0fb3eb32e44 100644
--- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
+++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
@@ -444,9 +444,9 @@  rte_pmd_init_internals(const char *name,
 	struct tpacket_req *req;
 	struct pkt_rx_queue *rx_queue;
 	struct pkt_tx_queue *tx_queue;
-	int rc, tpver, discard, bypass;
+	int rc, qsockfd, tpver, discard;
 	unsigned int i, q, rdsize;
-	int qsockfd, fanout_arg;
+	int fanout_arg __rte_unused, bypass __rte_unused;
 
 	for (k_idx = 0; k_idx < kvlist->count; k_idx++) {
 		pair = &kvlist->pairs[k_idx];
@@ -519,9 +519,13 @@  rte_pmd_init_internals(const char *name,
 	sockaddr.sll_protocol = htons(ETH_P_ALL);
 	sockaddr.sll_ifindex = (*internals)->if_index;
 
+#if defined(PACKET_FANOUT)
 	fanout_arg = (getpid() ^ (*internals)->if_index) & 0xffff;
-	fanout_arg |= (PACKET_FANOUT_HASH | PACKET_FANOUT_FLAG_DEFRAG |
-	               PACKET_FANOUT_FLAG_ROLLOVER) << 16;
+	fanout_arg |= (PACKET_FANOUT_HASH | PACKET_FANOUT_FLAG_DEFRAG) << 16;
+#if defined(PACKET_FANOUT_FLAG_ROLLOVER)
+	fanout_arg |= PACKET_FANOUT_FLAG_ROLLOVER << 16;
+#endif
+#endif
 
 	for (q = 0; q < nb_queues; q++) {
 		/* Open an AF_PACKET socket for this queue... */
@@ -553,6 +557,7 @@  rte_pmd_init_internals(const char *name,
 			goto error;
 		}
 
+#if defined(PACKET_QDISC_BYPASS)
 		bypass = 1;
 		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_QDISC_BYPASS,
 				&bypass, sizeof(bypass));
@@ -563,6 +568,7 @@  rte_pmd_init_internals(const char *name,
 			        pair->value);
 			goto error;
 		}
+#endif
 
 		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_RX_RING, req, sizeof(*req));
 		if (rc == -1) {
@@ -623,6 +629,7 @@  rte_pmd_init_internals(const char *name,
 			goto error;
 		}
 
+#if defined(PACKET_FANOUT)
 		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_FANOUT,
 				&fanout_arg, sizeof(fanout_arg));
 		if (rc == -1) {
@@ -631,6 +638,7 @@  rte_pmd_init_internals(const char *name,
 				"for %s\n", name, pair->value);
 			goto error;
 		}
+#endif
 	}
 
 	/* reserve an ethdev entry */