On 12/13/2019 2:05 PM, Ilya Maximets wrote: >> Hi sorry for late reply, didn't get this email. >> >> On 03.12.2019 16:16, Ilya Maximets wrote: >> > From: Ilya Maximets <i.maximets at samsung.com> >> > On 03.12.2019 14:45, Roi Dayan wrote: >> > > From: Paul Blakey <paulb at mellanox.com> >> > >
[...] >> >> We are calling dpif_set_features, not accessing dpif internals. > Not here, but you're dereferencing dpif pointer later producing the build > failure: > lib/netdev-offload-tc.c:1371:64: error: using member 'dpif_class' in > incomplete struct dpif > >> The thing is, that this sets a feature and not just probes for a feature. >> >> It enables a static branch on the kernel side which we might not want to >> enable any time, > Does it have any significant performance impact? Looking at the kernel > code I don't see any special heavy operations. I don't see any heavy operations as well, it was just suggested by mailing list, We just lookup the skb extension on vport recv. For normal packets this will just incur a simple inline check of skb_ext_find (which just does 'return skb->active_extensions & (1 << id)'), which will return false. > >> and an offloading a recirc() action was our hook to know that this is >> wanted, as we talked >> >> about in the kernel patch. > I do not know what you've talked about while upstreaming kernel parts, > but current patch-set for OVS doesn't work for several reasons: > > 1. Obviously, it doesn't build successfully, because netdev-offload > implementation now requires access to the internals of 'struct dpif'. Right I will check that. > > 2. You're using ovsthread_once to not enable feature twice and this will > break CT offloading if you'll remove datapath and create it back. You > will not be able to enable feature for the newly created datapath. Thanks, will fix that. > > 3. offloading module doesn't depend on datapath and could be theoretically > used from the userspace datapath at the same time and if userspace > datapath will reach feature enabling code first it will try to set > datapath features, will fail and kernel datapath will never try to > enable feature because of the same ovsthread_once. > > So, taking above issues into account, feature enabling should definitely > happen on datapath level and might or might not be controlled by the > ofproto. > >> I'm not sure how to do that from ofproto layer. > We may not involve the ofproto layer. For example, you may try to enable > the feature in dpif_netlink_open() and fallback if not supported. While > doing that you may remember the status of this feature and then send the > status to netdev-offload module via 'struct offload_info' or check directly > by implementing dpif_get_[user_]features(). As the performance impact is very minor, I'm for enabling/querying this feature at the start (probably via dpif_set_features) after a successfully creation in dpif_netlink_open of dpif-netlink, will that be ok with you? Thanks, Paul. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev