| Message ID | 1424916612-744-5-git-send-email-eyal.birger@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
> > sll->sll_halen = dev_parse_header(skb, sll->sll_addr); > > - PACKET_SKB_CB(skb)->origlen = skb->len; > + *PACKET_SKB_ORIGLEN(skb) = skb->len; > > if (pskb_trim(skb, snaplen)) > goto drop_n_acct; > > skb_set_owner_r(skb, sk); > - skb->dev = NULL; > skb_dst_drop(skb); There is a problem here. skb->dev must still be valid on the call to skb_set_owner_r(). Sorry about that. I'll spin a second version along with any other comments raised on this series. Eyal. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 25, 2015 at 9:10 PM, Eyal Birger <eyal.birger@gmail.com> wrote: > As part of an effort to move skb->dropcount to skb->cb[], 4 bytes > of additional room are needed in skb->cb[] in packet sockets. > > Store the skb original length in skb->dev instead of skb->cb[] for > this purpose. Another option is to delay preparation of the full sockaddr_ll struct until when it's needed. It often is not used at all (if msg_name == NULL). sll_family, sll_protocol and sll_pkttype can be derived when needed in packet_recvmsg. The first two are the head of sockaddr_ll, so a shorter struct filled in in packet_rcv only from sll_ifindex onwards would save the 4B needed for origlen. Actually, a union is simpler: struct __packet_cb_sockaddr_ll { union { struct { unsigned short sll_family; __be16 sll_protocol; } unsigned int sll_origlen; } /* etc.. */ }; The family and protocol can just overwrite sll_origlen in packet_recvmsg before the memcpy into msg->msg_name. Also interesting would be to avoid copying the address in the fast path as it may never be used, especially for long addresses (INFINIBAND_ALEN and FWNET_ALEN). From what I can tell, all but one header_ops.parse implementations return bytes from inside the packet, so could take an unsigned char ** as argument, store that pointer and postpone the memcpy to packet_recvmsg (handling skb_trim correctly). The exception is fwnet_header_parse, which would require some workaround. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Feb 26, 2015 at 7:26 PM, Willem de Bruijn <willemb@google.com> wrote: > On Wed, Feb 25, 2015 at 9:10 PM, Eyal Birger <eyal.birger@gmail.com> wrote: >> As part of an effort to move skb->dropcount to skb->cb[], 4 bytes >> of additional room are needed in skb->cb[] in packet sockets. >> >> Store the skb original length in skb->dev instead of skb->cb[] for >> this purpose. > > Another option is to delay preparation of the full sockaddr_ll struct until > when it's needed. It often is not used at all (if msg_name == NULL). > > sll_family, sll_protocol and sll_pkttype can be derived when needed > in packet_recvmsg. The first two are the head of sockaddr_ll, so > a shorter struct filled in in packet_rcv only from sll_ifindex onwards > would save the 4B needed for origlen. Actually, a union is simpler: > > struct __packet_cb_sockaddr_ll { > union { > struct { > unsigned short sll_family; > __be16 sll_protocol; > } > unsigned int sll_origlen; > } > /* etc.. */ > }; > > The family and protocol can just overwrite sll_origlen in packet_recvmsg > before the memcpy into msg->msg_name. > That's an interesting option. My personal inclination is not to inline sockaddr_ll in the packet cb structure. IMHO it would probably make sense to do it as part of your below suggestion. > Also interesting would be to avoid copying the address in the fast path > as it may never be used, especially for long addresses (INFINIBAND_ALEN > and FWNET_ALEN). From what I can tell, all but one header_ops.parse > implementations return bytes from inside the packet, so could take an > unsigned char ** as argument, store that pointer and postpone the > memcpy to packet_recvmsg (handling skb_trim correctly). The exception is > fwnet_header_parse, which would require some workaround. I think it may be interesting to pursue this option, though I don't think it should be part of this specific effort. Thanks! Eyal. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 26, 2015 at 1:38 PM, Eyal Birger <eyal.birger@gmail.com> wrote: > Hi, > > On Thu, Feb 26, 2015 at 7:26 PM, Willem de Bruijn <willemb@google.com> wrote: >> On Wed, Feb 25, 2015 at 9:10 PM, Eyal Birger <eyal.birger@gmail.com> wrote: >>> As part of an effort to move skb->dropcount to skb->cb[], 4 bytes >>> of additional room are needed in skb->cb[] in packet sockets. >>> >>> Store the skb original length in skb->dev instead of skb->cb[] for >>> this purpose. >> >> Another option is to delay preparation of the full sockaddr_ll struct until >> when it's needed. It often is not used at all (if msg_name == NULL). >> >> sll_family, sll_protocol and sll_pkttype can be derived when needed >> in packet_recvmsg. The first two are the head of sockaddr_ll, so >> a shorter struct filled in in packet_rcv only from sll_ifindex onwards >> would save the 4B needed for origlen. Actually, a union is simpler: >> >> struct __packet_cb_sockaddr_ll { >> union { >> struct { >> unsigned short sll_family; >> __be16 sll_protocol; >> } >> unsigned int sll_origlen; >> } >> /* etc.. */ >> }; >> >> The family and protocol can just overwrite sll_origlen in packet_recvmsg >> before the memcpy into msg->msg_name. >> > > That's an interesting option. > My personal inclination is not to inline sockaddr_ll in the packet cb structure. It already is, isn't it? > IMHO it would probably make sense to do it as part of your below suggestion. > >> Also interesting would be to avoid copying the address in the fast path >> as it may never be used, especially for long addresses (INFINIBAND_ALEN >> and FWNET_ALEN). From what I can tell, all but one header_ops.parse >> implementations return bytes from inside the packet, so could take an >> unsigned char ** as argument, store that pointer and postpone the >> memcpy to packet_recvmsg (handling skb_trim correctly). The exception is >> fwnet_header_parse, which would require some workaround. > > I think it may be interesting to pursue this option, though I don't > think it should > be part of this specific effort. Absolutely. This is a lot more work. Too much to fold into this patchset. I was more speculating in general. I may look into this at some point. > > Thanks! > Eyal. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 26, 2015 at 8:51 PM, Willem de Bruijn <willemb@google.com> wrote: > On Thu, Feb 26, 2015 at 1:38 PM, Eyal Birger <eyal.birger@gmail.com> wrote: >> Hi, >> >> On Thu, Feb 26, 2015 at 7:26 PM, Willem de Bruijn <willemb@google.com> wrote: >>> >>> Another option is to delay preparation of the full sockaddr_ll struct until >>> when it's needed. It often is not used at all (if msg_name == NULL). >>> >>> sll_family, sll_protocol and sll_pkttype can be derived when needed >>> in packet_recvmsg. The first two are the head of sockaddr_ll, so >>> a shorter struct filled in in packet_rcv only from sll_ifindex onwards >>> would save the 4B needed for origlen. Actually, a union is simpler: >>> >>> struct __packet_cb_sockaddr_ll { >>> union { >>> struct { >>> unsigned short sll_family; >>> __be16 sll_protocol; >>> } >>> unsigned int sll_origlen; >>> } >>> /* etc.. */ >>> }; >>> >>> The family and protocol can just overwrite sll_origlen in packet_recvmsg >>> before the memcpy into msg->msg_name. >>> >> >> That's an interesting option. >> My personal inclination is not to inline sockaddr_ll in the packet cb structure. > > It already is, isn't it? > I meant I prefer not to break out sockaddr_ll's individual fields in packet_skb_cb. Sorry for being ambiguous. >> IMHO it would probably make sense to do it as part of your below suggestion. >> >>> Also interesting would be to avoid copying the address in the fast path >>> as it may never be used, especially for long addresses (INFINIBAND_ALEN >>> and FWNET_ALEN). From what I can tell, all but one header_ops.parse >>> implementations return bytes from inside the packet, so could take an >>> unsigned char ** as argument, store that pointer and postpone the >>> memcpy to packet_recvmsg (handling skb_trim correctly). The exception is >>> fwnet_header_parse, which would require some workaround. >> >> I think it may be interesting to pursue this option, though I don't >> think it should >> be part of this specific effort. > > Absolutely. This is a lot more work. Too much to fold into this patchset. > I was more speculating in general. I may look into this at some point. > >> >> Thanks! >> Eyal. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 9c28cec..26889fd 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -216,7 +216,6 @@ static void prb_fill_vlan_info(struct tpacket_kbdq_core *, static void packet_flush_mclist(struct sock *sk); struct packet_skb_cb { - unsigned int origlen; union { struct sockaddr_pkt pkt; struct sockaddr_ll ll; @@ -224,6 +223,7 @@ struct packet_skb_cb { }; #define PACKET_SKB_CB(__skb) ((struct packet_skb_cb *)((__skb)->cb)) +#define PACKET_SKB_ORIGLEN(__skb) ((unsigned long *)&(__skb)->dev) #define GET_PBDQC_FROM_RB(x) ((struct tpacket_kbdq_core *)(&(x)->prb_bdqc)) #define GET_PBLOCK_DESC(x, bid) \ @@ -1825,13 +1825,12 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, sll->sll_halen = dev_parse_header(skb, sll->sll_addr); - PACKET_SKB_CB(skb)->origlen = skb->len; + *PACKET_SKB_ORIGLEN(skb) = skb->len; if (pskb_trim(skb, snaplen)) goto drop_n_acct; skb_set_owner_r(skb, sk); - skb->dev = NULL; skb_dst_drop(skb); /* drop conntrack reference */ @@ -3006,7 +3005,7 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock, aux.tp_status = TP_STATUS_USER; if (skb->ip_summed == CHECKSUM_PARTIAL) aux.tp_status |= TP_STATUS_CSUMNOTREADY; - aux.tp_len = PACKET_SKB_CB(skb)->origlen; + aux.tp_len = *PACKET_SKB_ORIGLEN(skb); aux.tp_snaplen = skb->len; aux.tp_mac = 0; aux.tp_net = skb_network_offset(skb);
As part of an effort to move skb->dropcount to skb->cb[], 4 bytes of additional room are needed in skb->cb[] in packet sockets. Store the skb original length in skb->dev instead of skb->cb[] for this purpose. Signed-off-by: Eyal Birger <eyal.birger@gmail.com> --- net/packet/af_packet.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)