hello Tom,
On Fri, 2017-04-07 at 08:43 -0700, Tom Herbert wrote:
> On Fri, Apr 7, 2017 at 7:16 AM, Davide Caratti <dcaratti@xxxxxxxxxx> wrote:
> > @@ -742,8 +744,10 @@ struct sk_buff {
> > __u8 csum_valid:1;
> > __u8 csum_complete_sw:1;
> > __u8 csum_level:2;
> > - __u8 __unused:1; /* one bit hole */
> > -
> > + enum {
> > + INTERNET_CHECKSUM = 0,
> > + CRC32C_CHECKSUM,
> > + } csum_algo:1;
>
> I am worried this opens the door to a new open ended functionality
> that will be rarely used in practice. Checksum offload is pervasive,
> CRC offload is still a very narrow use case.
thank you for the prompt response. I thought there was a silent
agreement on that - Alexander proposed usage of an enum bitfield to be
ready for FCoE (and I'm not against it, unless I have to find a second
free bit in struct sk_buff :-) ). But maybe I'm misunderstanding your
concern: is it the name of the variable, (csum_algo instead of
crc32c_csum), or the usage of enum bitfield (or both?) ?
On Fri, 2017-04-07 at 08:43 -0700, Tom Herbert wrote:
> Adding yet more
> CRC/checksum variants will need more bits. It may be sufficient for
> now just to make this a single bit which indicates "ones' checksum" or
> indicates "other". In this case of "other" we need some analysis so
> determine which checksum it is, this might be something that flow
> dissector could support.
... which is my intent: by the way, from my perspective, we don't need more than 1 bit
to extend the functionality. While reviewing my code, I was also considering
extending the witdth of skb->ip_summed from 2 to 3 bit, so that it was possible
to
#define
CRC32C_PARTIAL <- for SCTP
CRC_PARTIAL <- for FCoE
CHECKSUM_PARTIAL <- for everything else
It's conceptually the same thing, and the free bit is used more
efficiently. But then I would need to check all places where
CHECKSUM_PARTIAL is used in assignments and test: so, I told myself it's
not worth doing it until somebody requests to extend this functionality to
FCoE.
> > @@ -3129,6 +3133,14 @@ struct skb_checksum_ops {
> >
> > extern const struct skb_checksum_ops *crc32c_csum_stub __read_mostly;
> >
> > +static inline void skb_set_crc32c_ipsummed(struct sk_buff *skb,
> > + const u8 ip_summed)
> > +{
> > + skb->csum_algo = ip_summed == CHECKSUM_PARTIAL ? CRC32C_CHECKSUM :
> > + INTERNET_CHECKSUM;
> > + skb->ip_summed = ip_summed;
>
> This seems odd to me. skb->csum_algo and skb->ip_summed always end up
> having the same value.
this is accidentally true for CHEKSUM_NONE and CHECKSUM_PARTIAL, and only
if skb carries a SCTP packet. This was my intent:
ip_summed (2 bit) | csum_algo (1 bit)
---------------------------------------+-------------------
CHEKSUM_NONE = 0 | INTERNET_CHECKSUM = 0
CHECKSUM_PARTIAL = 1 | CRC32C_CHECKSUM = 1
CHECKSUM_COMPLETE = 2 (not applicable) | INTERNET_CHECKSUM = 0 (don't care)
CHECKSUM_UNNECESSARY = 3 | INTERNET_CHECKSUM = 0
I can do this in a more explicit way, changing the prototype to
static inline void skb_set_crc32c_ipsummed(struct sk_buff *skb,
const u8 ip_summed,
const u8 csum_algo)
(with the advantage of saving a test on the value of ip_summed).
Find in the comment below the reason why I'm clearing csum_algo every time
the SCTP CRC32c is computed.
> > --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > @@ -81,7 +81,7 @@ static void sctp_nat_csum(struct sk_buff *skb, sctp_sctphdr_t *sctph,
> > unsigned int sctphoff)
> > {
> > sctph->checksum = sctp_compute_cksum(skb, sctphoff);
> > - skb->ip_summed = CHECKSUM_UNNECESSARY;
> > + skb_set_crc32c_ipsummed(skb, CHECKSUM_UNNECESSARY);
>
> The old code is better. CHECKSUM_UNNECESSARY already applies to non IP
> checksums. There is nothing special about crc32 in this regard and
> skb->csum_algo should only be valid when skb->ip_summed ==
> CHECKSUM_PARTIAL so no need to set it here. This point should also be
> in documentation.
In my understanding, csum_algo needs to be set to INTERNET_CHECKSUM after the
CRC32c is computed. Otherwise, after subsequent operation on the skb (e.g. it
is encapsulated in a UDP frame), there is the possibility for skb->ip_summed
to become CHECKSUM_PARTIAL again. So, to ensure that skb_checksum_help() and
not skb_crc32c_help() will be called, csum_algo must be 0.
To minimize the impact of the patch, I substituted all assignments of skb->ip_summed,
done by SCTP-related code, with calls to skb_set_crc32c_ipsummed(). The alternative is
to explicitly set csum_algo to 0 (INTERNET_CHECKSUM) in SCTP-related code. Do you agree?
thank you in advance,
regards
--
davide
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
