On 15.12.2019 14:35, Paul Blakey wrote: > > 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?
I'm not against enabling by default if it doesn't cause performance degradation. Just be sure that we could fall back if the feature is not supported by kernel. BTW, while offloading it might be better to use 'struct offload_info' to pass the feature support status. This way you'll not need to operate with/dereference dpif pointers. > > Thanks, > > Paul. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
