lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 5 Jul 2013 10:09:59 +0200 From: Willy Tarreau <w@....eu> To: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com> Cc: netdev@...r.kernel.org, Gregory CLEMENT <gregory.clement@...e-electrons.com>, Eric Dumazet <edumazet@...gle.com> Subject: Re: [PATCH] net: convert mvneta to build_skb() On Fri, Jul 05, 2013 at 09:50:38AM +0200, Thomas Petazzoni wrote: > > In practice we don't really need to store the frag_size in the struct, we > > just need to know if the data were allocated using netdev_alloc_frag() > > or kmalloc() to know how to free them. So a single bit would be enough, > > and I thought about just doing a +1 on the pointer when we need to free > > using kmalloc(). But that would add unneeded extra work in the fast path > > to fix the pointers. And since we need to pass frag_size to build_skb() > > it was a reasonable solution in my opinion. > > Aah, okay makes sense. So now, the question that comes up is why this > skb_size calculation is done in every call of mvneta_rx_refill() ? It > should be computed once, at the same time pkt_size is calculated, and > stored in the mvneta_port structure? Then you just need to test whether > it is smaller or larger than PAGE_SIZE to decide whether to use > netdev_alloc_frag() vs. kmalloc(). > > I.e, I would turn all the: > > if (pp->frag_size) > ... > else > ... > > into: > > if (pp->skb_size <= PAGE_SIZE) > ... > else > ... I was concerned by the same thing and saw that tg3 does the same. Then I realized that we're only using constants here, so there isn't much calculation (basically it's skb_size = pkt_size + something). However it would probably make the code less confusing, especially if we were two different readers concerned about this frag_size not changing per packet. > Of course, you can always hide this test behind a small macro or inline > function, to make it something like: > > if (mvneta_uses_small_skbs(pp)) > ... > else > ... > > A better name than mvneta_uses_small_skbs() would of course be useful. > > > > For example, in mvneta_rx_refill(), you store the skb_size in > > > pp->frag_size, and then you later re-use it in mvneta_rxq_drop_pkts. > > > What guarantees you that mvneta_rx_refill() hasn't be called in the > > > mean time for a different packet, in a different rxq, for the same > > > network interface, and the value of pp->frag_size has been overridden? > > > > It's not a problem since the refill() applies pp->pkt_size which doesn't > > change between calls. It's only changed in mvneta_change_mtu() which > > first stops the device. So I think it's safe. > > Yeah, sure, now that I see that pp->frag_size is constant for a given > MTU configuration, it looks safe. But I find the current code that > retests and reassigns pp->frag_size at every call of rxq_refill() to be > very confusing. Yes I agree. I think I'll experiment with the +/-1 on the pointer to see the impact on the rest of the code, because probably that with a few macros we could make that more transparent this way. Best regards, Willy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists