[Intel-wired-lan] [PATCH 6/9] igc: Add Initial XDP support

Andre Guedes andre.guedes at intel.com
Thu Oct 22 18:04:49 UTC 2020


Hi Maciej,

Quoting Maciej Fijalkowski (2020-10-22 02:53:10)
> > > > @@ -1932,24 +1939,37 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> > > >  
> > > >               rx_buffer = igc_get_rx_buffer(rx_ring, size);
> > > >  
> > > > -             if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
> > > > -                     void *pktbuf = page_address(rx_buffer->page) +
> > > > -                                    rx_buffer->page_offset;
> > > > +             pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
> > > >  
> > > > +             if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
> > > >                       timestamp = igc_ptp_rx_pktstamp(q_vector->adapter,
> > > >                                                       pktbuf);
> > > >                       pkt_offset = IGC_TS_HDR_LEN;
> > > >                       size -= IGC_TS_HDR_LEN;
> > > >               }
> > > >  
> > > > -             /* retrieve a buffer from the ring */
> > > > -             if (skb)
> > > > +             if (!skb) {
> > > > +                     struct igc_adapter *adapter = q_vector->adapter;
> > > > +
> > > > +                     xdp.data = pktbuf + pkt_offset;
> > > > +                     xdp.data_end = xdp.data + size;
> > > > +                     xdp.data_hard_start = pktbuf - igc_rx_offset(rx_ring);
> > > 
> > > This needs some fixing. You're saying that build_skb() can't be currently
> > > used which means that the headroom given for XDP will always be zero.
> > > 
> > > You need to either teach igc_rx_offset to return XDP_PACKET_HEADROOM for
> > > case when ring is not using build_skb() or make build_skb() usable.
> > 
> > The xdp headroom doesn't seem to be required to support XDP_PASS, XDP_DROP and
> > XDP_ABORTED actions, which is the goal of this patch. That was the rationale to
> > not change igc_rx_offset() in this patch.
> > 
> > The igc_rx_offset() helper is updated in the next patch which adds support for
> > XDP_TX action since it requires xdp headroom.
> 
> Sorry when I wrote that I haven't looked at next patch.
> The thing is that you can have a bpf program that will do the
> encapsulation and return XDP_PASS for example. For encapsulation you need
> to provide a headroom.

Understood, thanks for the example. I was not aware of the encapsulation +
XDP_PASS case. I'll fix this in the v2.

> > > > @@ -3881,6 +3901,11 @@ static int igc_change_mtu(struct net_device *netdev, int new_mtu)
> > > >       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> > > 
> > > I think I was mentioning it with igb xdp review, shouldn't we take double
> > > vlan header into account?
> > 
> > Good question. It seems like we should take it into consideration when updating
> > adapter->max_frame_size later in the igc_change_mtu(). However, note that in
> > the code added below, we check 'new_mtu', not 'max_frame', so with or without
> > double tagging it should do the right thing.
> 
> Hm yeah but I think it should be fixed anyway even without xdp being in
> picture?

Agreed. I can provide a separate patch soon.

Cheers,
Andre


More information about the Intel-wired-lan mailing list