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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Oct 21, 2020 at 03:16:51PM -0700, Andre Guedes wrote:
> 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.

Right, but va/size misorder was the thing that caught my attention.

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

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

Hm yeah but I think it should be fixed anyway even without xdp being in
picture?

> 
> > > +     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