[lkml]   [2009]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: iwlagn: order 2 page allocation failures
    On Wed, Sep 09, 2009 at 01:05:38PM -0700, reinette chatre wrote:
    > Mel and Frans,
    > Thank you very much for digging into this.
    > On Wed, 2009-09-09 at 09:55 -0700, Mel Gorman wrote:
    > > Conceivably a better candidate for this problem is commit
    > > 4752c93c30441f98f7ed723001b1a5e3e5619829 introduced in May 2009. If there
    > > are less than RX_QUEUE_SIZE/2 left, it starts replenishing buffers. Mohamed,
    > > is it absolutly necessary it use GFP_ATOMIC there? If an allocation fails,
    > > does it always mean frames are dropped or could it just replenish what it
    > > can and try again later printing a warning only if allocation failures are
    > > resulting in packet loss?
    > I agree that this patch may be the reason we are seeing this issue. We
    > would like to keep using GFP_ATOMIC here, but it is not necessary for an
    > allocation failure to be so noisy since the function doing the
    > allocation (iwl_rx_allocate) is always followed by a call to
    > iwl_rx_queue_restock which will schedule a refill if the buffers are
    > running low.

    Right, so it's a "refill now if you can and defer further refilling
    until later".

    > We can thus use ___GFP_NOWARN for the allocations in
    > iwl_rx_allocate and leave it to the restocking to find the needed memory
    > when it tried its allocations using GFP_KERNEL.

    Would it be possible to use __GFP_NOWARN *unless* this allocation is
    necessary to receive the packet?

    > I do think it is useful to let user know about these allocation
    > failures, even if it does not result in packet loss. Wrapping it in
    > net_ratelimit() will help though.

    If it does not distinguish between failures causing packet loss and just a
    temporary issue, I'd be worried the messages would generate bug reports and
    we genuinely won't know if it's a real problem or not.

    As a total aside, there is still the problem that the driver is depending on
    order-2 allocations. On systems without swap, the allocation problem could be
    more severe as there are fewer pages the system can use to regain contiguity.

    > How about the patch below?
    > diff --git a/drivers/net/wireless/iwlwifi/iwl-rx.c b/drivers/net/wireless/iwlwifi/iwl-rx.c
    > index b90adcb..f0ee72e 100644
    > --- a/drivers/net/wireless/iwlwifi/iwl-rx.c
    > +++ b/drivers/net/wireless/iwlwifi/iwl-rx.c
    > @@ -252,10 +252,11 @@ void iwl_rx_allocate(struct iwl_priv *priv, gfp_t priority)
    > /* Alloc a new receive buffer */
    > skb = alloc_skb(priv->hw_params.rx_buf_size + 256,
    > - priority);
    > + priority | __GFP_NOWARN);

    So, would it be possible here to only remove __GFP_NOWARN if this is GFP_ATOMIC
    (implying we have to refill now) and the rxq->free_count is really small
    e.g. <= 2?

    > if (!skb) {
    > - IWL_CRIT(priv, "Can not allocate SKB buffers\n");
    > + if (net_ratelimit())
    > + IWL_CRIT(priv, "Can not allocate SKB buffer.\n");

    Similarly, could the message either be supressed when there is enough
    buffers in the RX queue or differenciate between enough buffers and
    things getting tight possibly causing packet loss?

    The IWL_CRIT() part even is a hint - there is no guarantee that the allocation
    failure is really a critical problem.

    > /* We don't reschedule replenish work here -- we will
    > * call the restock method and if it still needs
    > * more buffers it will schedule replenish */
    > diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
    > index 0909668..5d9fb78 100644
    > --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
    > +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
    > @@ -1147,10 +1147,10 @@ static void iwl3945_rx_allocate(struct iwl_priv *priv, gfp_t priority)
    > spin_unlock_irqrestore(&rxq->lock, flags);
    > /* Alloc a new receive buffer */
    > - skb = alloc_skb(priv->hw_params.rx_buf_size, priority);
    > + skb = alloc_skb(priv->hw_params.rx_buf_size, priority | __GFP_NOWARN);
    > if (!skb) {
    > if (net_ratelimit())
    > - IWL_CRIT(priv, ": Can not allocate SKB buffers\n");
    > + IWL_CRIT(priv, "Can not allocate SKB buffer.\n");
    > /* We don't reschedule replenish work here -- we will
    > * call the restock method and if it still needs
    > * more buffers it will schedule replenish */

    Mel Gorman
    Part-time Phd Student Linux Technology Center
    University of Limerick IBM Dublin Software Lab

     \ /
      Last update: 2009-09-10 11:05    [W:0.025 / U:0.248 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site