Zoltan, original l3 userspace patch set gets ether_type from packet->md.
eh->eth_type = packet->md.packet_ethertype; I think we should keep struct ovs_action_push_eth consistent in both kernel and userspace, we can use the way in original l3 userspace patch set to get ether_type. -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Zoltán Balogh Sent: Thursday, May 4, 2017 6:02 PM To: Ben Pfaff <[email protected]>; Jiri Benc ([email protected]) <[email protected]> Cc: '[email protected]' <[email protected]> Subject: Re: [ovs-dev] [PATCH v4 2/7] userspace: Support for push_eth and pop_eth actions > On Tue, Apr 25, 2017 at 04:30:23PM +0000, Zoltán Balogh wrote: > > From: Jan Scheurich <[email protected]> > > > > Add support for actions push_eth and pop_eth to the netdev datapath > > and the supporting libraries. This patch relies on the support for > > these actions in the kernel datapath to be present. > > Hi, can you help me to understand this part here? I believe that this > will lead to userspace sending a OVS_ACTION_ATTR_PUSH_ETH action > argument to the kernel that is different from what the kernel actually > wants. What's the idea, and how is that problem avoided? > > struct ovs_action_push_eth { > struct ovs_key_ethernet addresses; > #ifndef __KERNEL__ > __be16 eth_type; > #endif > }; > > Thanks, > > Ben. Hi, I'm afraid the kernel would not accept that OVS_ACTION_ATTR_PUSH_ETH action in this case. Jiri, could you confirm, please? We could remove the eth_type from struct ovs_action_push_eth, then put an additional "set action" after putting "push_eth" in the odp_put_push_eth_action() function in order to set the ethertype of the packet. void odp_put_push_eth_action(struct ofpbuf *odp_actions, const struct eth_addr *eth_src, const struct eth_addr *eth_dst, const ovs_be16 eth_type) { struct ovs_action_push_eth eth; memset(ð, 0, sizeof eth); if (eth_src) { eth.addresses.eth_src = *eth_src; } if (eth_dst) { eth.addresses.eth_dst = *eth_dst; } nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_PUSH_ETH, ð, sizeof eth); commit_set_action(odp_actions, OVS_KEY_ATTR_ETHERTYPE, eth_type); } That way we would split the push_eth action into a simpler push_eth and a set_field. However this would lead to modify odp_execute_set_action()as well, since changing of ethertype with set_field is not allowed: static void odp_execute_set_action(struct dp_packet *packet, const struct nlattr *a) { enum ovs_key_attr type = nl_attr_type(a); const struct ovs_key_ipv4 *ipv4_key; const struct ovs_key_ipv6 *ipv6_key; struct pkt_metadata *md = &packet->md; switch (type) { ... case OVS_KEY_ATTR_ETHERTYPE: case OVS_KEY_ATTR_IN_PORT: case OVS_KEY_ATTR_VLAN: case OVS_KEY_ATTR_TCP_FLAGS: case OVS_KEY_ATTR_CT_STATE: case OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4: case OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6: case OVS_KEY_ATTR_CT_ZONE: case OVS_KEY_ATTR_CT_MARK: case OVS_KEY_ATTR_CT_LABELS: case __OVS_KEY_ATTR_MAX: default: OVS_NOT_REACHED(); } } I guess something similar should be done at kernel side too, since the kernel module would not accept set_field ethertype either. Another option would be to convert OVS_ACTION_ATTR_PUSH_ETH action argument between userspace and kernel. Do you have any other proposal? Btw, the original comment message of struct ovs_action_push_eth indicates eth_type as a second member. /* * struct ovs_action_push_eth - %OVS_ACTION_ATTR_PUSH_ETH action argument. * @addresses: Source and destination MAC addresses. * @eth_type: Ethernet type */ struct ovs_action_push_eth { struct ovs_key_ethernet addresses; }; Best regards, Zoltan _______________________________________________ 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
