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: Thu, 2 Feb 2017 16:22:23 -0800 From: Jakub Kicinski <kubakici@...pl> To: Michael Chan <michael.chan@...adcom.com> Cc: David Miller <davem@...emloft.net>, Netdev <netdev@...r.kernel.org> Subject: Re: [PATCH net-next v2 01/12] bnxt_en: Refactor rx SKB function. On Thu, 2 Feb 2017 15:40:19 -0800, Michael Chan wrote: > On Thu, Feb 2, 2017 at 2:56 PM, Jakub Kicinski <kubakici@...pl> wrote: > > On Thu, 2 Feb 2017 11:55:29 -0500, Michael Chan wrote: > >> @@ -755,8 +757,8 @@ static void bnxt_reuse_rx_agg_bufs(struct bnxt_napi *bnapi, u16 cp_cons, > >> > >> static struct sk_buff *bnxt_rx_skb(struct bnxt *bp, > >> struct bnxt_rx_ring_info *rxr, u16 cons, > >> - u16 prod, u8 *data, dma_addr_t dma_addr, > >> - unsigned int len) > >> + u16 prod, void *data, dma_addr_t dma_addr, > >> + unsigned int offset_and_len) > >> { > >> int err; > >> struct sk_buff *skb; > >> @@ -776,7 +778,7 @@ static struct sk_buff *bnxt_rx_skb(struct bnxt *bp, > >> } > >> > >> skb_reserve(skb, BNXT_RX_OFFSET); > >> - skb_put(skb, len); > >> + skb_put(skb, offset_and_len & 0xffff); > >> return skb; > >> } > >> > > > > Sorry to be a pain but I still don't understand (a) why you make this > > change in the first patch if it's only needed from patch 5 on; > > Because I'm thinking ahead. The function is now a function pointer > and I want to make the parameter change now. You're thinking ahead, reviewers have to guess ahead :) > > (b) why > > do you encode the two parameters in a single u32? It's the seventh > > parameter so it's going on the stack anyway, no? > > Both the length and the offset come from the hardware's rx completion > record. Both are u16. The offset happens to be in the upper 16-bit > in the hardware record. So it is convenient to encode it like this > and I chose to do it like this. Of course, using a separate parameter > will also work. Yes, I initially thought you read them out this way straight from the descriptor but you actually combine them into this form: + unsigned int payload_len; + + payload_len = (le32_to_cpu(rxcmp->rx_cmp_misc_v1) & + RX_CMP_PAYLOAD_OFFSET) | len; + skb = bp->rx_skb_func(bp, rxr, cons, prod, data, dma_addr, I don't mind though, I was just hoping this is some clever optimization technique I could learn :)
Powered by blists - more mailing lists