On 02/16/2014 12:00 PM, Patrick McHardy wrote: > On Sun, Feb 16, 2014 at 11:49:12AM +0100, Nikolay Aleksandrov wrote: >> On 02/16/2014 11:36 AM, Patrick McHardy wrote: >>>> >>>> netfilter: nf_tables: nft_meta module get/set ops >>>> >>>> That patch is similar to what you propose, but it sets the meta fields >>>> of a packet. >>> >>> Actually I'd propose two different init functions, that's just not pretty. >>> >> Hm, okay. >> How about something else, since I wanted to make use of the inlined payload >> fast op, couldn't I just break the dreg/sreg in separate variables and >> based on whether sreg is set act in the fast op (i.e. get/set based on >> that) ? That way we can save some code duplication and keep the ops as >> they're. (That'll work for the slow op as well actually) > > I don't agree to adding a set fast op. The get fast op is meant to be > small since its the most common case and is inlined into the main loop. > Anything added there needs a *really* good justification. Modifying > packet data isn't a very common operation and should be kept seperate. > > Outside of the main loop, there's no need for a fast op as well since > memcpy *is* fast and any optimized implementation will already do the > same thing you do. > Right, okay going the standard way then :-) >> Also, there's a small problem for payload because the code in the >> select_ops function: >> if (len <= 4 && IS_ALIGNED(offset, len) && base != >> NFT_PAYLOAD_LL_HEADER) >> return &nft_payload_fast_ops; >> else >> return &nft_payload_ops; >> >> Has a problem when the offset ends in 101b and length of 3 is used, then >> the fast ops get selected but since that case isn't handled there, we'll >> only load 1 byte from the offset, which is fine for loading since we can >> just switch to 4 bytes and mask out later the unneeded byte when comparing >> for example, but for writing it's a problem since someone might actually >> want to write out 3 bytes. Of course one can always add 2 expressions (1 >> byte + 2 byte write) :-) > > Good catch, we should make sure the offset is a power of two since the > fast version is only intended for well aligned loads. > > Would you care to send a patch? Sure, I'll take care of it. > -- > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
