On Thu, Jan 16, 2014 at 03:09:04PM -0800, Ben Pfaff wrote:
> On Wed, Jan 15, 2014 at 05:17:02PM +0900, Simon Horman wrote:
> > If VLAN acceleration is used when the kernel receives a packet
> > then the outer-most VLAN tag will not be present in the packet
> > when it is received by netdev-linux. Rather, it will be present
> > in auxdata.
> >
> > This patch uses recvmsg() instead of recv() to read auxdata for
> > each packet and if the vlan_tid is set then it is added to the packet.
> >
> > Adding the vlan_tid makes use of headroom available
> > in the buffer parameter of rx_recv.
> >
> > Co-authored-by: Ben Pfaff <b...@nicira.com>
> > Signed-off-by: Simon Horman <ho...@verge.net.au>
>
> I noticed one minor bug (ETH_ADDR_LEN should be ETH_HEADER_LEN) and a
> number of opportunities for minor improvements. I'm folding in the
> following, and then I'll apply it to master.
Thanks, looks good.
>
> Thank you!
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 4bd4bfc..9c1a36d 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -809,9 +809,7 @@ netdev_linux_rx_construct(struct netdev_rx *rx_)
> }
>
> val = 1;
> - error = setsockopt(rx->fd, SOL_PACKET, PACKET_AUXDATA,
> - &val, sizeof val);
> - if (error) {
> + if (setsockopt(rx->fd, SOL_PACKET, PACKET_AUXDATA, &val, sizeof
> val)) {
> error = errno;
> VLOG_ERR("%s: failed to mark socket for auxdata (%s)",
> netdev_get_name(netdev_), ovs_strerror(error));
> @@ -946,9 +944,9 @@ netdev_linux_rx_recv_sock(int fd, struct ofpbuf *buffer)
> continue;
> }
>
> - aux = (struct tpacket_auxdata *)(void *)CMSG_DATA(cmsg);
> + aux = ALIGNED_CAST(struct tpacket_auxdata *, CMSG_DATA(cmsg));
> if (auxdata_has_vlan_tci(aux)) {
> - if (retval < ETH_ADDR_LEN) {
> + if (retval < ETH_HEADER_LEN) {
> return EINVAL;
> }
>
> @@ -990,11 +988,9 @@ netdev_linux_rx_recv(struct netdev_rx *rx_, struct
> ofpbuf *buffer)
> retval = (rx->is_tap
> ? netdev_linux_rx_recv_tap(rx->fd, buffer)
> : netdev_linux_rx_recv_sock(rx->fd, buffer));
> - if (retval) {
> - if (retval != EAGAIN && retval != EMSGSIZE) {
> - VLOG_WARN_RL(&rl, "error receiving Ethernet packet on %s: %s",
> - ovs_strerror(errno), netdev_rx_get_name(rx_));
> - }
> + if (retval && retval != EAGAIN && retval != EMSGSIZE) {
> + VLOG_WARN_RL(&rl, "error receiving Ethernet packet on %s: %s",
> + ovs_strerror(errno), netdev_rx_get_name(rx_));
> }
>
> return retval;
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 66b411a..8e62421 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -500,6 +500,13 @@ netdev_parse_name(const char *netdev_name_, char **name,
> char **type)
> }
> }
>
> +/* Attempts to open a netdev_rx handle for obtaining packets received on
> + * 'netdev'. On success, returns 0 and stores a nonnull 'netdev_rx *' into
> + * '*rxp'. On failure, returns a positive errno value and stores NULL into
> + * '*rxp'.
> + *
> + * Some kinds of network devices might not support receiving packets. This
> + * function returns EOPNOTSUPP in that case.*/
> int
> netdev_rx_open(struct netdev *netdev, struct netdev_rx **rxp)
> OVS_EXCLUDED(netdev_mutex)
> @@ -531,6 +538,7 @@ netdev_rx_open(struct netdev *netdev, struct netdev_rx
> **rxp)
> return error;
> }
>
> +/* Closes 'rx'. */
> void
> netdev_rx_close(struct netdev_rx *rx)
> OVS_EXCLUDED(netdev_mutex)
> @@ -543,6 +551,29 @@ netdev_rx_close(struct netdev_rx *rx)
> }
> }
>
> +/* Attempts to receive a packet from 'rx' into the tailroom of 'buffer',
> which
> + * must initially be empty. If successful, returns 0 and increments
> + * 'buffer->size' by the number of bytes in the received packet, otherwise a
> + * positive errno value.
> + *
> + * Returns EAGAIN immediately if no packet is ready to be received.
> + *
> + * Returns EMSGSIZE, and discards the packet, if the received packet is
> longer
> + * than 'ofpbuf_tailroom(buffer)'.
> + *
> + * Implementations may make use of VLAN_HEADER_LEN bytes of tailroom to
> + * add a VLAN header which is obtained out-of-band to the packet. If
> + * this occurs then VLAN_HEADER_LEN bytes of tailroom will no longer be
> + * available for the packet, otherwise it may be used for the packet
> + * itself.
> + *
> + * It is advised that the tailroom of 'buffer' should be
> + * VLAN_HEADER_LEN bytes longer than the MTU to allow space for an
> + * out-of-band VLAN header to be added to the packet. At the very least,
> + * 'buffer' must have at least ETH_TOTAL_MIN bytes of tailroom.
> + *
> + * This function may be set to null if it would always return EOPNOTSUPP
> + * anyhow. */
> int
> netdev_rx_recv(struct netdev_rx *rx, struct ofpbuf *buffer)
> {
> @@ -563,12 +594,15 @@ netdev_rx_recv(struct netdev_rx *rx, struct ofpbuf
> *buffer)
> }
> }
>
> +/* Arranges for poll_block() to wake up when a packet is ready to be received
> + * on 'rx'. */
> void
> netdev_rx_wait(struct netdev_rx *rx)
> {
> rx->netdev->netdev_class->rx_wait(rx);
> }
>
> +/* Discards any packets ready to be received on 'rx'. */
> int
> netdev_rx_drain(struct netdev_rx *rx)
> {
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
