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

Andre Guedes andre.guedes at intel.com
Wed Oct 21 22:16:51 UTC 2020


Hi Maciej,

Thank you for your review.

Quoting Maciej Fijalkowski (2020-10-21 07:23:49)
> On Thu, Oct 08, 2020 at 07:53:46PM -0700, Andre Guedes wrote:
> > This patch adds the initial XDP support to the igc driver. For now,
> > only XDP_PASS, XDP_DROP, XDP_ABORT actions are supported. Upcoming
> 
> s/XDP_ABORT/XDP_ABORTED

Good catch, I'll fix it.

> > @@ -1592,11 +1593,11 @@ static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring,
> >  
> >  static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
> >                                        struct igc_rx_buffer *rx_buffer,
> > -                                      unsigned int size, int pkt_offset,
> > +                                      struct xdp_buff *xdp,
> >                                        ktime_t timestamp)
> >  {
> > -     void *va = page_address(rx_buffer->page) + rx_buffer->page_offset +
> > -                pkt_offset;
> > +     void *va = xdp->data;
> > +     unsigned int size = xdp->data_end - xdp->data;
> >       unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size);
> >       unsigned int headlen;
> >       struct sk_buff *skb;
> 
> RCT

We can move 'va' declaration around, but note that 'size' is utilized to
calculate 'truesize' so the RCT won't be perfect still.

> > @@ -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.

> > +                     xdp_set_data_meta_invalid(&xdp);
> 
> What about xdp.frame_sz?

Same as above.

> > @@ -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.

> > +     if (igc_xdp_is_enabled(adapter) && new_mtu > ETH_DATA_LEN) {
> > +             netdev_dbg(netdev, "Jumbo frames not supported with XDP");
> > +             return -EINVAL;
> > +     }
> > +

[...]

> > +struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter,
> > +                              struct xdp_buff *xdp)
> > +{
> > +     struct bpf_prog *prog;
> > +     int res;
> > +     u32 act;
> > +
> > +     rcu_read_lock();
> > +
> > +     prog = READ_ONCE(adapter->xdp_prog);
> > +     if (!prog)
> > +             goto unlock;
> 
> res will be uninitialized if you hit this goto.

Good catch, I'll fix it.

Best,
Andre


More information about the Intel-wired-lan mailing list