On Wed, Mar 11, 2020 at 1:22 PM Flavio Leitner <[email protected]> wrote: > > On Wed, Mar 11, 2020 at 02:13:07PM +0100, Ilya Maximets wrote: > > On 3/11/20 1:37 PM, Flavio Leitner wrote: > > > On Wed, Mar 11, 2020 at 01:30:32PM +0100, Ilya Maximets wrote: > > >> On 3/11/20 1:13 PM, Flavio Leitner wrote: > > >>> On Wed, Mar 11, 2020 at 11:47:22AM +0100, Ilya Maximets wrote: > > >>>> On 3/10/20 10:01 PM, Flavio Leitner wrote: > > >>>>> > > >>>>> Hi William, > > >>>>> > > >>>>> Any chance to get the function comments copied to the non-dpdk > > >>>>> versions as well? > > >>>>> > > >>>>> I wonder if we could define DP_PACKET_OL.* defines as it is > > >>>>> proposed in this patch if DPDK is not enabled, or define them > > >>>>> as DPDK defines. For example: > > >>>>> > > >>>>> #ifdef DPDK > > >>>>> DP_PACKET_OL_TX_TCP_SEG = PKT_TX_TCP_SEG > > >>>>> #else > > >>>>> DP_PACKET_OL_TX_TCP_SEG = 0x40 > > >>>>> #endif > > >>>>> > > >>>>> Then > > >>>>> struct dp_packet > > >>>>> #ifdef DPDK_NETDEV > > >>>>> struct rte_mbuf mbuf; /* DPDK mbuf */ > > >>>>> #define ol_flags mbuf.ol_flags > > >>>>> #else > > >>>>> void *base_; /* First byte of allocated space. */ > > >>>>> uint16_t allocated_; /* Number of bytes allocated. */ > > >>>>> uint16_t data_ofs; /* First byte actually in use. */ > > >>>>> uint32_t size_; /* Number of bytes in use. */ > > >>>>> uint32_t ol_flags; /* Offloading flags. */ > > >>>>> > > >>>>> > > >>>>> > > >>>>> Then we would have a single: > > >>>>> static inline bool > > >>>>> dp_packet_hwol_is_tso(const struct dp_packet *b) > > >>>>> { > > >>>>> return !!(b->ol_flags & PKT_TX_TCP_SEG); > > >>>>> } > > >>>>> > > >>>>> I haven't tried, so don't know if I am missing something or if it > > >>>>> will look ugly, but it would save us many function copies. > > >>>> > > >>>> > > >>>> Interesting. I generally like the idea except the ' #define ol_flags' > > >>>> part. We should define something that could not be used accidentally, > > >>>> e.g. > > >>>> > > >>>> #ifdef DPDK_NETDEV > > >>>> #define DP_PACKET_OL_FLAGS_FIELD mbuf.ol_flags > > >>>> #else > > >>>> #define DP_PACKET_OL_FLAGS_FIELD ol_flags > > >>>> #endif > > >>>> ... > > >>>> return !!(b->DP_PACKET_OL_FLAGS_FIELD & PKT_TX_TCP_SEG); > > >>>> > > >>>> > > >>>> Or, better write the inline access function that should look much > > >>>> more clean: > > >>>> > > >>>> static inline uint32_t * > > >>>> dp_packet_ol_flags_get(const struct dp_packet *b) > > >>>> { > > >>>> #ifdef DPDK_NETDEV > > >>>> return &b->mbuf.ol_flags; > > >>>> #else > > >>>> return &b->ol_flags; > > >>>> #endif > > >>>> } > > >>>> ... > > >>>> return !!(*dp_packet_ol_flags_get(b) & PKT_TX_TCP_SEG); > > >>>> ... > > >>>> *dp_packet_ol_flags_get(b) |= PKT_TX_SCTP_CKSUM; > > >>>> ... > > >>>> > > >>>> > > >>>> What do you think? > > >>> > > >>> Then we will have cases like this just to set another flag: > > >>> dp_packet_ol_flags_set(b, dp_packet_ol_flags_get(b) | FLAG); > > >>> > > >>> I suspect that the define you proposed is better or the original > > >>> one. > > >> > > >> I didn't propose the _set() function. See example of flag setting > > >> few lines above. > > > > > > I meant that we would need a 'set' either by a function/define or > > > other unusual way. IMO, '*dp_packet_ol_flags_get(b) |= ' isn't usual. > > > > I don't insist. Just a note that such constructions are not very unusual. > > In OVS coverage counters and per-thread static data implemented this way. > > There are a lot of examples in Linux kernel, e.g. per_cpu_ptr(). > > '_get()' might be replaced with '_ptr()' for readability, though. > > Definitely the _ptr() makes a difference. > > Another approach is using union: > struct dp_packet { > #ifdef DPDK > struct mbuf mbuf; > #else > ... > union { > int ol_flags; > } mbuf; > ... > #endif > }; > > Then we always have: > packet->mbuf.ol_flags = 0x10;
This might mislead people that DPDK mbuf is being used. > > I don't have a strong opinion. > Thanks, > -- > fbl Hi Flavio and Ilya, Thanks a lot for these valuable feedbacks. I will work on the next version. William _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
